Re: AIO v2.5

Hi,

On 2025-04-01 08:11:59 -0700, Noah Misch wrote:
> On Mon, Mar 31, 2025 at 08:41:39PM -0400, Andres Freund wrote:
> > updated version
>
> All non-write patches (1-7) are ready for commit, though I have some cosmetic
> recommendations below. I've marked the commitfest entry Ready for Committer.

Thanks!

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.

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.

> This comment is a copy of the previous test's comment. While the comment is
> not false, consider changing it to:
>
> # Check one read reporting multiple invalid blocks.

> > + # Then test zeroing vio zero_damaged_pages
>
> s/vio/via/
>

These make sense.

> > +# 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".

> > + 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.

> 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.
*/

> > + /*
> > + * The underlying IO actually completed OK, and thus the "invalid"
> > + * portion of the IOV actually contains valid data. That can hide
> > + * a lot of problems, e.g. if we were to wrongly mark a buffer,
> > + * that wasn't read according to the shortened-read, IO as valid,
> > + * the contents would look valid and we might miss a bug.
>
> Minimally s/read, IO/read IO,/ but I'd edit a bit further:
>
> * a lot of problems, e.g. if we were to wrongly mark-valid a
> * buffer that wasn't read according to the shortened-read IO, the
> * contents would look valid and we might miss a bug.

Adopted.

> > Subject: [PATCH v2.15 05/18] md: Add comment & assert to buffer-zeroing path
> > in md[start]readv()
>
> > The zero_damaged_pages path is incomplete, as as missing segments are not
>
> s/as as/as/
>
> > For now, put an Assert(false) comments documenting this choice into mdreadv()
>
> s/comments/and comments/
>
> > + * For PG 18, we are putting an Assert(false) in into
> > + * mdreadv() (triggering failures in assertion-enabled builds,
>
> s/in into/in/

> > Subject: [PATCH v2.15 06/18] aio: comment polishing
>
> > + * - Partial reads need to be handle by the caller re-issuing IO for the
> > + * unread blocks
>
> s/handle/handled/

All adopted. I'm sorry that you had to see so much of tiredness-enhanced
dyslexia :(.

Greetings,

Andres Freund