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