Message-ID: <491ba86b-6294-37bb-fe00-c91e829dfc82@enterprisedb.com>
Date: Mon, 24 Apr 2023 23:20:32 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: Missing update of all_hasnulls in BRIN opclasses
Content-Language: en-US
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
 Matthias van de Meent <boekewurm+postgres@gmail.com>,
 pgsql-hackers@lists.postgresql.org
References: <20230424153648.cyaifmcchslbcsrf@alvherre.pgsql>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <20230424153648.cyaifmcchslbcsrf@alvherre.pgsql>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/491ba86b-6294-37bb-fe00-c91e829dfc82%40enterprisedb.com>
Precedence: bulk

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