Date: Tue, 1 Apr 2025 09:07:27 -0700
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Robert Haas <robertmhaas@gmail.com>,
	Jakub Wartak <jakub.wartak@enterprisedb.com>,
	Jelte Fennema-Nio <postgres@jeltef.nl>,
	Antonin Houska <ah@cybertec.at>
Subject: Re: AIO v2.5
Message-ID: <20250401160727.bd.nmisch@google.com>
References: <wkjj4p2rmkevutkwc6tewoovdqznj6c6nvjmvii4oo5wmbh5sr@retq7d6uqs4j>
 <ym3dqpa4xcvoeknewcw63x77vnqdosbqcetjinb2zfoh65k55m@m4ozmwhr6lk6>
 <20250329134143.ca.nmisch@google.com>
 <d4ff3qxudxn2ze3zk2xwhds5ysezfei3uqeuimymj624t5chsd@sgr2h3rmln5t>
 <wouqlwmsxbgak4kow7p4s6g4voh2bai7b4acy324v4ncgh2nnq@5r5o6nvghsko>
 <20250329212929.a6.nmisch@google.com>
 <usbwzckj7q3jhfx3ann3nrfnukmupbs35axvq5zfyeo6nvrzrm@onjhxs2du4st>
 <7s6fclfekpcxoaaorwrq67v4vmgixf2dcjcgznuj7vxs3ie3wq@okqbi5vabqbx>
 <20250401151159.51.nmisch@google.com>
 <kas47e54inkpgnirnr3txm5zywxkjobhbx6466sfaolmkjtnlu@xuucqvkchksl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <kas47e54inkpgnirnr3txm5zywxkjobhbx6466sfaolmkjtnlu@xuucqvkchksl>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250401160727.bd.nmisch%40google.com>
Precedence: bulk

On Tue, Apr 01, 2025 at 11:55:20AM -0400, Andres Freund wrote:
> On 2025-04-01 08:11:59 -0700, Noah Misch wrote:
> > On Mon, Mar 31, 2025 at 08:41:39PM -0400, Andres Freund wrote:

> I haven't yet pushed the changes, but will work on that in the afternoon.
> 
> I plan to afterwards close the CF entry and will eventually create a new one
> for write support, although probably only rebasing onto
> https://postgr.es/m/stj36ea6yyhoxtqkhpieia2z4krnam7qyetc57rfezgk4zgapf%40gcnactj4z56m
> and addressing some of the locking issues.

Sounds good.

> WRT the locking issues, I've been wondering whether we could make
> LWLockWaitForVar() work that purpose, but I doubt it's the right approach.
> Probably better to get rid of the LWLock*Var functions and go for the approach
> I had in v1, namely a version of LWLockAcquire() with a callback that gets
> called between LWLockQueueSelf() and PGSemaphoreLock(), which can cause the
> lock acquisition to abort.

What are the best thing(s) to read to understand the locking issues?

> > > +# Verify checksum handling when creating database from an invalid database.
> > > +# This also serves as a minimal check that cross-database IO is handled
> > > +# reasonably.
> >
> > To me, "invalid database" is a term of art from the message "cannot connect to
> > invalid database".  Hence, I would change "invalid database" to "database w/
> > invalid block" or similar, here and below.  (Alternatively, just delete "from
> > an invalid database".  It's clear from the context.)
> 
> Yea, I agree, this is easy to misunderstand when stepping back. I went for "with
> an invalid block".

Sounds good.

> > > +	if (corrupt_checksum)
> > > +	{
> > > +		bool		successfully_corrupted = 0;
> > > +
> > > +		/*
> > > +		 * Any single modification of the checksum could just end up being
> > > +		 * valid again. To be sure
> > > +		 */
> >
> > Unfinished sentence.
> 
> Oops. See below.
> 
> 
> > That said, I'm not following why we'd need this loop.  If this test code
> > were changing the input to the checksum, it's true that an input bit flip
> > might reach the same pd_checksum.  The test case is changing pd_checksum,
> > not the input bits.
> 
> We might be changing the input, due to the zero/corrupt_header options. Or we
> might be called on a page that is *already* corrupted. I did encounter that
> situation once while writing tests, where the tests only passed if I made the
> + 1 a + 2. Which was, uh, rather confusing and left me feel like I was cursed
> that day.

Got it.

> > I don't see how changing pd_checksum could leave the
> > page still valid.  There's only one valid pd_checksum value for a given
> > input page.
> 
> I updated the comment to:
> 		/*
> 		 * Any single modification of the checksum could just end up being
> 		 * valid again, due to e.g. corrupt_header changing the data in a way
> 		 * that'd result in the "corrupted" checksum, or the checksum already
> 		 * being invalid. Retry in that, unlikely, case.
> 		 */

Works for me.