On 11/29/23 15:52, Tomas Vondra wrote:
>> ...
>>
>> This also made me think a bit more about how we're working with the
>> tuples. With your latest patch, we always deserialize and re-serialize
>> the sorted brin tuples, just in case the next tuple will also be a
>> BRIN tuple of the same page range. Could we save some of that
>> deserialization time by optimistically expecting that we're not going
>> to need to merge the tuple and only store a local copy of it locally?
>> See attached 0002; this saves some cycles in common cases.
>>
>
> Good idea!
>
FWIW there's a bug, in this part of the optimization:
------------------
+ if (memtuple == NULL)
+ memtuple = brin_deform_tuple(state->bs_bdesc, btup,
+ memtup_holder);
+
union_tuples(state->bs_bdesc, memtuple, btup);
continue;
------------------
The deforming should use prevbtup, otherwise union_tuples() jut combines
two copies of the same tuple.
Which however brings me to the bigger issue with this - my stress test
found this issue pretty quickly, but then I spent quite a bit of time
trying to find what went wrong. I find this reworked code pretty hard to
understand, and not necessarily because of how it's written. The problem
is it the same loop tries to juggle multiple pieces of information with
different lifespans, and so on. I find it really hard to reason about
how it behaves ...
I did try to measure how much it actually saves, but none of the tests I
did actually found measurable improvement. So I'm tempted to just not
include this part, and accept that we may deserialize some of the tuples
unnecessarily.
Did you actually observe measurable improvements in some cases?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company