Subject: Re: Missing update of all_hasnulls in BRIN opclasses

From: Tomas Vondra <[email-masked]>
Date: Mon, 24 Apr 2023 23:20:32 +0200

On 4/24/23 17:36, Alvaro Herrera wrote:
> On 2023-Apr-23, Tomas Vondra wrote:
> 
>> here's an updated version of the patch, including a backport version. I
>> ended up making the code yet a bit closer to master by introducing
>> add_values_to_range(). The current pre-14 code has the loop adding data
>> to the BRIN tuple in two places, but with the "fixed" logic handling
>> NULLs and the empty_range flag the amount of duplicated code got too
>> high, so this seem reasonable.
> 
> In backbranches, the new field to BrinMemTuple needs to be at the end of
> the struct, to avoid ABI breakage.
> 

Good point.

> There's a comment in add_values_to_range with a typo "If the range was had".
> Also, "So we should not see empty range that was not modified" should
> perhaps be "should not see an empty range".
> 

OK, I'll check the wording of the comments.

> (As for your FIXME comment in brin_memtuple_initialize, I think you're
> correct: we definitely need to reset bt_placeholder.  Otherwise, we risk
> places that call it in a loop using a BrinMemTuple with one range with
> the flag set, in a range where that doesn't hold.  It might be
> impossible for this to happen, given how narrow the conditions are on
> which bt_placeholder is used; but it seems safer to reset it anyway.)
> 

Yeah. But isn't that a separate preexisting issue, strictly speaking?

> Some pgindent noise would be induced by this patch.  I think it's worth
> cleaning up ahead of time.
> 

True. Will do.

> I did a quick experiment of turning the patches over -- applying test
> first, code fix after (requires some conflict fixing).  With that I
> verified that the behavior of 'hasnulls' indeed changes with the code
> fix.
> 

Thanks. Could you do some testing of the union_tuples stuff too? It's a
bit tricky part - the behavior is timing sensitive, so testing it
requires gdb breakpoints breakpoints or something like that. I think
it's correct, but it'd be nice to check that.

regards

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

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 7 May 2023 00:13:07 +0200

>> 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)

b) encoding the info in existing BrinMemTuple flags - e.g. we could use
bt_placeholder to store two bits, not just one. Seems a bit ugly.

c) ignore the issue - AFAICS this would be an issue only for (external)
code accessing BrinMemTuple structs, but I don't think we're aware of
any out-of-core BRIN opclasses or anything like that ...

regards

---

From: Julien Rouhaud <[email-masked]>
Date: Sun, 7 May 2023 13:08:59 +0800

Hi,

> any out-of-core BRIN opclasses or anything like that ...

FTR there's at least postgis that implments BRIN opclasses (for geometries and
geographies), but there's nothing fancy in the implementation and it doesn't
access BrinMemTuple struct.

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 7 May 2023 14:50:51 +0200

> access BrinMemTuple struct.

Right. I believe that should be fine, because opclasses don't access the
tuple directly - instead we pass pointers to individual pieces. But
maybe it'd be a good idea to test this.

regards