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