Re: Dereferenced pointers checked as NULL in btree_utils_var.c

On 01/21/2015 07:14 AM, Michael Paquier wrote:
> Also,
> looking at the code of gist, gbt_var_same is called through
> gistKeyIsEQ, which is used for an insertion or for a split. The point
> is that when doing an insertion, a call to gistgetadjusted is done and
> we look if one of the keys is NULL. If one of them is, code does not
> go through gistKeyIsEQ, so the NULL checks do not seem necessary in
> gbt_var_same.

I think you are confusing NULL pointers with an SQL NULL.
gistgetadjusted checks that it's not an SQL NULL (!oldisnull[i]), but it
does not check if it's a NULL pointer
(DatumGetPointer(oldentries[i].key) != NULL). The case we're worried
about is that the value is not an SQL NULL, i.e. isnull flag is false,
but the Datum is a NULL pointer.

Nevertheless, looking at the code, I don't that a NULL pointer can ever
be passed to the same-method, for any of the built-in or contrib
opclasses, but it's very confusing because some functions check for
that. My proof goes like this:

1. The key value passed as argument must've been originally formed by
the compress, union, or picksplit methods.

1.1. Some compress methods do nothing, and just return the passed-in
key, which comes from the table and cannot be a NULL pointer (the
compress method is never called for SQL NULLs). Other compress methods
don't check for a NULL pointer, and would crash if there was one.
gist_poly_compress() and gist_circle_compress() do check for a NULL, but
they only return a NULL key if the input key is NULL, which cannot
happen because the input comes from a table. So I believe the
NULL-checks in those functions are dead code, and none of the compress
methods ever return a NULL key.

1.2. None of the union methods return a NULL pointer (nor expect one as
input).

1.3. None of the picksplit methods return a NULL pointer. They all
return one of the original values, or one formed with the union method.
The picksplit method can return a NULL pointer in the spl_ldatum or
spl_rdatum field, in the degenerate case that it puts all keys on the
same halve. However, the caller, gistUserPickSplit checks for that and
immediately overwrites spl_ldatum and spl_rdatum with sane values in
that case.

I don't understand what this sentence in the documentation on the
compress method means:

> Depending on your needs, you could also need to care about
> compressing NULL values in there, storing for example (Datum) 0 like
> gist_circle_compress does.

The compress method is never called for NULLs, so the above is nonsense.
I think it should be removed, as well as the checks in
gist_circle_compress and gist_poly_compress. According to git history,
the check in gist_circle_compress been there ever since the module was
imported into contrib/rtree_gist, in 2001. The documentation was added
later:

commit a0a3883dd977d6618899ccd14258a0696912a9d2
Author: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Date: Fri Jun 12 19:48:53 2009 +0000

Improve documentation about GiST opclass support functions.
Dimitri Fontaine

I'm guessing that Tom added that sentence (it was not in the patch that
Dimitri submitted originally) just because there was that check in the
existing function, without realizing that the check was in fact dead code.

- Heikki