Re: Missing update of all_hasnulls in BRIN opclasses

On 10/21/22 17:50, Matthias van de Meent wrote:
> On Fri, 21 Oct 2022 at 17:24, Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> Hi,
>>
>> While working on some BRIN code, I discovered a bug in handling NULL
>> values - when inserting a non-NULL value into a NULL-only range, we
>> reset the all_nulls flag but don't update the has_nulls flag. And
>> because of that we then fail to return the range for IS NULL ranges.
> 
> Ah, that's bad.
> 

Yeah, I guess we'll need to inform the users to consider rebuilding BRIN
indexes on NULL-able columns.

> One question though: doesn't (shouldn't?) column->bv_allnulls already
> imply column->bv_hasnulls? The column has nulls if all of the values
> are null, right? Or is the description of the field deceptive, and
> does bv_hasnulls actually mean "has nulls bitmap"?
> 

What null bitmap do you mean? We're talking about summary for a page
range - IIRC we translate this to nullbitmap for a BRIN tuple, but there
may be multiple columns, and "has nulls bitmap" is an aggregate over all
of them.

Yeah, maybe it'd make sense to also have has_nulls=true whenever
all_nulls=true, and maybe it'd be simpler because it'd be enough to
check just one flag in consistent function etc. But we still need to
track 2 different states - "has nulls" and "has summary".

In any case, this ship sailed long ago - at least for the existing
opclasses.


>> Attached is a patch fixing this by properly updating the has_nulls flag.
> 
> One comment on the patch:
> 
>> +SET enable_seqscan = off;
>> + [...]
>> +SET enable_seqscan = off;
> 
> Looks like duplicated SETs. Should that last one be RESET instead?
> 

Yeah, should have been RESET.

> Apart from that, this patch looks good.
> 

Thanks!

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company