Re: Missing update of all_hasnulls in BRIN opclasses

On 5/15/23 12:06, Alvaro Herrera wrote:
> On 2023-May-07, Tomas Vondra wrote:
> 
>>> Álvaro wrote:
>>>> In backbranches, the new field to BrinMemTuple needs to be at the end of
>>>> the struct, to avoid ABI breakage.
>>
>> Unfortunately, this is not actually possible :-(
>>
>> The BrinMemTuple has a FLEXIBLE_ARRAY_MEMBER at the end, so we can't
>> place anything after it. I think we have three options:
>>
>> a) some other approach? - I really can't see any, except maybe for going
>> back to the previous approach (i.e. encoding the info using the existing
>> BrinValues allnulls/hasnulls flags)
> 
> Actually, mine was quite the stupid suggestion: the BrinMemTuple already
> has a 3 byte hole in the place where you originally wanted to add the
> flag:
> 
> struct BrinMemTuple {
>     _Bool                      bt_placeholder;       /*     0     1 */
> 
>     /* XXX 3 bytes hole, try to pack */
> 
>     BlockNumber                bt_blkno;             /*     4     4 */
>     MemoryContext              bt_context;           /*     8     8 */
>     Datum *                    bt_values;            /*    16     8 */
>     _Bool *                    bt_allnulls;          /*    24     8 */
>     _Bool *                    bt_hasnulls;          /*    32     8 */
>     BrinValues                 bt_columns[];         /*    40     0 */
> 
>     /* size: 40, cachelines: 1, members: 7 */
>     /* sum members: 37, holes: 1, sum holes: 3 */
>     /* last cacheline: 40 bytes */
> };
> 
> so putting it there was already not causing any ABI breakage.  So, the
> solution to this problem of not being able to put it at the end is just
> to return the struct to your original formulation.
> 

Thanks, that's pretty lucky. It means we're not breaking on-disk format
nor ABI, which is great.

Attached is a final version of the patches - I intend to do a bit more
testing, go through the comments once more, and get this committed today
or perhaps tomorrow morning, so that it gets into beta1.

Unfortunately, while polishing the patches, I realized union_tuples()
has yet another long-standing bug with handling NULL values, because it
does this:

    /* Adjust "hasnulls". */
    if (!col_a->bv_hasnulls && col_b->bv_hasnulls)
        col_a->bv_hasnulls = true;

but let's assume "col_a" is a summary representing "1" and "col_b"
represents NULL (col_b->bv_hasnulls=false col_b->bv_allnulls=true).
Well, in that case we fail to "remember" col_a should represent NULL
values too :-(

This is somewhat separate issue, because it's unrelated to empty ranges
(neither of the two ranges is empty). It's hard to test it, because it
requires a particular timing of the concurrent actions, but a breakpoint
in brin.c on the brin_can_do_samepage_update call (in summarize_range)
does the trick for manual testing.

0001 fixes the issue. 0002 is the original fix, and 0003 is just the
pageinspect changes (for master only).

For the backbranches, I thought about making the code more like master
(by moving some of the handling from opclasses to brin.c), but decided
not to. It'd be low-risk, but it feels wrong to kinda do what the master
does under "oi_regular_nulls" flag.


regards

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