Re: Missing update of all_hasnulls in BRIN opclasses

On 1/9/23 00:34, Tomas Vondra wrote:
> 
> I've been working on this over the past couple days, trying to polish
> and commit it over the weekend - both into master and backbranches.
> Sadly, the backpatching part turned out to be a bit more complicated
> than I expected, because of the BRIN reworks in PG14 (done by me, as
> foundation for the new opclasses, so ... well).
> 
> Anyway, I got it done, but it's a bit uglier than I hoped for and I
> don't feel like pushing this on Sunday midnight. I think it's correct,
> but maybe another pass to polish it a bit more is better.
> 
> So here are two patches - one for 11-13, the other for 14-master.
> 

I spent a bit more time on this fix. I realized there are two more
places that need fixes.

Firstly, the placeholder tuple needs to be marked as "empty" too, so
that it can be correctly updated by other backends etc.

Secondly, union_tuples had a couple bugs in handling empty ranges (this
is related to the placeholder tuple changes). I wonder what's the best
way to test this in an automated way - it's very dependent on timing of
the concurrent updated. For example we need to do something like this:

    T1: run pg_summarize_range() until it inserts the placeholder tuple
    T2: do an insert into the page range (updates placeholder)
    T1: continue pg_summarize_range() to merge into the placeholder

But there are no convenient ways to do this, I think. I had to check the
various cases using breakpoints in gdb etc.

I'm not very happy with the union_tuples() changes - it's quite verbose,
perhaps a bit too verbose. We have to check for empty ranges first, and
then various combinations of allnulls/hasnulls flags for both BRIN
tuples. There are 9 combinations, and the current code just checks them
one by one - I was getting repeatedly confused by the original code, but
maybe it's too much.

As for the backpatch, I tried to keep it as close to the 14+ fixes as
possible, but it effectively backports some of the 14+ BRIN changes. In
particular, 14+ moved most of the NULL-handling logic from opclasses to
brin.c, and I think it's reasonable to do that for the backbranches too.

The alternative is to apply the same fix to every BRIN_PROCNUM_UNION
opclass procedure out there. I guess doing that for minmax+inclusion is
not a huge deal, but what about external opclasses? And without the fix
the indexes are effectively broken. Fixing this outside in brin.c (in
the union procedure) fixes this for every opclass procedure, without any
actual limitation of functinality (14+ does that anyway).

But maybe someone thinks this is a bad idea and we should do something
else in the backbranches?

regards

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