Feedback-ID: ia2694551:Fastmail
Date: Mon, 24 Apr 2023 17:36:48 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Missing update of all_hasnulls in BRIN opclasses
Message-ID: <20230424153648.cyaifmcchslbcsrf@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <24ef13a1-b798-0a5f-33c0-e6cf9e65492a@enterprisedb.com>
Archived-At: <https://www.postgresql.org/message-id/20230424153648.cyaifmcchslbcsrf%40alvherre.pgsql>
Precedence: bulk

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)