Re: Dereferenced pointers checked as NULL in btree_utils_var.c

On Tue, Jan 20, 2015 at 11:38 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> Coverity is pointing out $subject, with the following stuff in gbt_var_same():
>> ...
>> As Heikki pointed me out on IM, the lack of crash report in this area,
>> as well as similar coding style in cube/ seem to be sufficient
>> arguments to simply remove those NULL checks instead of doing more
>> solid checks on them. Patch is attached.
>
> The way to form a convincing argument that these checks are unnecessary
> would be to verify that (1) the SQL-accessible functions directly calling
> gbt_var_same() are all marked STRICT, and (2) the core GIST code never
> passes a NULL to these support functions. I'm prepared to believe that
> (1) and (2) are both true, but it merits checking.

Sure. gbt_var_same is called by those functions in btree_gist/:
- gbt_bit_same
- gbt_bytea_same
- gbt_numeric_same
- gbt_text_same
=# select proname, proisstrict from pg_proc
where proname in ('gbt_bit_same', 'gbt_bytea_same',
'gbt_numeric_same', 'gbt_text_same');
proname | proisstrict
------------------+-------------
gbt_text_same | t
gbt_bytea_same | t
gbt_numeric_same | t
gbt_bit_same | t
(4 rows)

For the second point, I have run regression tests with an assertion in
gbt_var_same checking if t1 or t2 are NULL and tests worked. 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.

Btw, looking at the code of gist, I think that it would be interesting
to add an assertion in gistKeyIsEQ like that:
Assert(DatumGetPointer(a) != NULL && DatumGetPointer(b) != NULL);
Thoughts?
--
Michael