Re: Missing update of all_hasnulls in BRIN opclasses - Mailing list pgsql-hackers
From | Alvaro Herrera |
---|---|
Subject | Re: Missing update of all_hasnulls in BRIN opclasses |
Date | |
Msg-id | 20230424153648.cyaifmcchslbcsrf@alvherre.pgsql Whole thread Raw |
In response to | Re: Missing update of all_hasnulls in BRIN opclasses (Tomas Vondra <tomas.vondra@enterprisedb.com>) |
Responses |
Re: Missing update of all_hasnulls in BRIN opclasses
Re: Missing update of all_hasnulls in BRIN opclasses |
List | pgsql-hackers |
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. 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". (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.) Some pgindent noise would be induced by this patch. I think it's worth cleaning up ahead of time. 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. > Both cases have a patch extending pageinspect to show the new flag, but > obviously we should commit that only in master. In backbranches it's > meant only to make testing easier. In backbranches, I think it should be reasonably easy to add a --1.7--1.7.1.sql file and set the default version to 1.7.1; that then enables us to have the functionality (and the tests) in older branches too. If you just add a --1.X.1--1.12.sql version to each branch that's identical to that branch's current pageinspect version upgrade script, that would let us have working upgrade paths for all branches. This is a bit laborious but straightforward enough. If you don't feel like adding that, I volunteer to add the necessary scripts to all branches after you commit the bugfix, and ensure that all upgrade paths work correctly. > I plan to do a bit more testing, I'd welcome some feedback - it's a > long-standing bug, and it'd be good to finally get this fixed. I don't > think the patch can be made any simpler. The approach looks good to me. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Oh, great altar of passive entertainment, bestow upon me thy discordant images at such speed as to render linear thought impossible" (Calvin a la TV)
pgsql-hackers by date: