Re: AIO v2.5 - Mailing list pgsql-hackers

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