On 10/22/22 10:00, Alvaro Herrera wrote:
> On 2022-Oct-22, Tomas Vondra wrote:
>
>> I wonder how to do this in a back-patchable way - we can't add
>> parameters to the opclass procedure, and the other solution seems to be
>> storing it right in the BrinMemTuple, somehow. But that's likely an ABI
>> break :-(
>
> Hmm, I don't see the ABI incompatibility. BrinMemTuple is an in-memory
> structure, so you can add new members at the end of the struct and it
> will pose no problems to existing code.
>
But we're not passing BrinMemTuple to the opclass procedures - we're
passing a pointer to BrinValues, so we'd have to add the flag there. And
we're storing an array of those, so adding a field may shift the array
even if you add it at the end. Not sure if that's OK or not.
>> The only solution I can think of is actually passing it using all_nulls
>> and has_nulls - we could set both flags to true (which we never do now)
>> and teach the opclass that it signifies "empty" (and thus not to update
>> has_nulls after resetting all_nulls).
>>
>> Something like the attached (I haven't added any more tests, not sure
>> what would those look like - I can't think of a query testing this,
>> although maybe we could check how the flags change using pageinspect).
>
> I'll try to have a look at these patches tomorrow or on Monday.
>
I was experimenting with this a bit more, and unfortunately the latest
patch is still a few bricks shy - it did fix this particular issue, but
there were other cases that remained/got broken. See the first patch,
that adds a bunch of pageinspect tests testing different combinations.
After thinking about it a bit more, I think we can't quite fix this at
the opclass level, so the yesterday's patches are wrong. Instead, this
should be fixed in values_add_to_range() - the whole trick is we need to
remember the range was empty at the beginning, and only set the flag
when allnulls is false.
The reworked patch does that. And we can use the same logic (both flags
set mean no tuples were added to the range) when building the index, a
separate flag is not needed.
This slightly affects existing regression tests, because we won't create
any ranges for empty table (now we created one, because we initialized a
tuple in brinbuild and then wrote it to disk). This means that
brin_summarize_range now returns 0, but I think that's fine.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
