Re: AIO v2.5

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.