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
