Re: Dereferenced pointers checked as NULL in btree_utils_var.c

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> 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.

Actually both of these deserve to be worried about; though it's fairly
clear from looking at the core GIST code that it avoids calling
gistKeyIsEQ on SQL NULLs.

> 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.

Sounds good to me.

> 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.

I believe you're right that I added this because there were checks for
null pointers in some but not all of the opclass support functions.
It looked to me like some opclasses might be intending to pass around null
pointers as valid (not-SQL-NULL) values. I think your analysis above
eliminates that idea though. It's a sufficiently weird concept that
I don't feel a need to document or support it.

So I'm fine with taking out both this documentation text and the dead
null-pointer checks; but let's do that all in one patch not piecemeal.
In any case, this is just cosmetic cleanup; there's no actual hazard
here.

regards, tom lane