MIME-Version: 1.0
References: <zljergweqti7x67lg5ije2rzjusie37nslsnkjkkby4laqqbfw@3p3zu522yykv>
 <CA+hUKGKwzPJNcxCFUswe0K=0e7rG79dvqt9o17-E3soxLPxF+Q@mail.gmail.com>
 <CAAKRu_atdnPCCy=kfxqWT62Ckaiz3G5t=S97tW24CuL3i3fFfQ@mail.gmail.com>
 <CAN55FZ17ScU--nGM8cjjPtwPMOMonKZ9vMehPcc2sOQNLVskvA@mail.gmail.com>
 <CAAKRu_ZM1epxTdt2=4-g4ff6UC09zne+0xg_gNv3d7LEcxEvNA@mail.gmail.com>
 <CAN55FZ0ymY=XfaHQXHKG+Mbit6BZzZ9d-UWb6dmRXYm_xCvfQg@mail.gmail.com>
 <CAAKRu_YV0D_9wWgGkHdWgQsooqmcc08Uqb3tqxzrwcDXHajDtA@mail.gmail.com>
 <u73un3xeljr4fiidzwi4ikcr6vm7oqugn4fo5vqpstjio6anl2@hph6fvdiiria>
 <CAAKRu_bCZfWpmfmfUiQjDgFK12T8FUox-hV9YFDy6XHTh=i0ew@mail.gmail.com>
 <prkj4fv7dvyew4lbad2g5l346nn6u66inndwx7nzzap5c3bdmh@fds54c2hx3yd>
 <42rdu4q44kvsq53fz5qgzuawqpaytvnemsnquynlfch5mqhc2m@6ytnlgivtzro>
In-Reply-To: <42rdu4q44kvsq53fz5qgzuawqpaytvnemsnquynlfch5mqhc2m@6ytnlgivtzro>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Fri, 20 Mar 2026 15:50:59 -0400
Message-ID: 
 <CAAKRu_YiaJgbmHGTFHRJEP9+AtVCt+gHTd1rmXm05agT_1Mx2A@mail.gmail.com>
Subject: Re: Don't synchronously wait for already-in-progress IO in read
 stream
To: Andres Freund <andres@anarazel.de>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>,
 Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers@postgresql.org, Peter Geoghegan <pg@bowt.ie>,
 Tomas Vondra <tv@fuzzy.cz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAAKRu_YiaJgbmHGTFHRJEP9%2BAtVCt%2BgHTd1rmXm05agT_1Mx2A%40mail.gmail.com>
Precedence: bulk

On Thu, Mar 19, 2026 at 6:22=E2=80=AFPM Andres Freund <andres@anarazel.de> =
wrote:
>
> Thinking about it more, I also got worried about the duplicating of
> logic. It's perhaps acceptable with the patches as-is, but we'll soon nee=
d
> something very similar for AIO writes. Then we'd end up with like 5 varia=
nts,
> because we'd still need the existing StartBufferIO() for some cases where=
 we
> do want to wait (e.g. the edge case in ExtendBufferedRelShared()).
>
> In the attached prototype I replaced your patch introducing
> PrepareHeadBufferReadIO()/ PrepareAdditionalBufferReadIO() with one that
> instead revises StartBufferIO() to have the enum return value you introdu=
ced
> and a PgAioWaitRef * argument that callers that would like to asynchronou=
sly
> wait for the IO to complete (others pass NULL). There are some other clea=
nups
> in it too, see the commit message for more details.

I've come around to this. The aspect I like least is that io_wref is
used both as an output parameter _and_ a decision input parameter.

Some callers never want to wait on in-progress IO (and don't have to),
while others must eventually wait but can defer that waiting as long
as they have a wait reference. If they can't get a wait reference,
they have no way to wait later, so they must wait now. The presence of
io_wref indicates this difference.

I think it's important to express that less mechanically than in your
current header comment and comment in the else block of
StartSharedBufferIO() where we do the waiting. Explaining first=E2=80=94bef=
ore
detailing argument combinations=E2=80=94why a caller would want to pass
io_wref might help.

However, I do think you need to enumerate the different combinations
of wait and io_wref (as you've done) to make it clear what they are.

I, for example, find it very confusing what wait =3D=3D false and io_wref
not NULL would mean. If IO is in progress on the buffer and the
io_wref is not valid yet, the caller would get the expected
BUFFER_IO_IN_PROGRESS return value but io_wref won't be set. I could
see callers easily misinterpreting the API and passing this
combination when what they want is wait =3D=3D true and io_wref not NULL
-- because they don't want to synchronously wait.

I don't have any good suggestions despite thinking about it, though.

Two other things about 0007:

    for (int i =3D nblocks_done + 1; i < operation->nblocks; i++)
    {
        /* Must be consecutive block numbers. */
        Assert(BufferGetBlockNumber(buffers[i - 1]) =3D=3D
               BufferGetBlockNumber(buffers[i]) - 1);

        status =3D StartBufferIO(buffers[nblocks_done], true, false, NULL);

Copy-paste bug above, should be StartBufferIO(buffers[i],...

I would mention that currently BUFFER_IO_IN_PROGRESS is not used in
the first StartBufferIO() case, so that is dead code as of this commit

> I also updated "Restructure AsyncReadBuffers()" to move
> pgstat_prepare_report_checksum_failure() and the computation of flags to
> before the ReadBuffersCanStartIO().  And added a comment explaining why l=
ittle
> should be added between the ReadBuffersCanStartIO() calls.
>
> Thoughts?

Yea, definitely think the comment is important.

- Melanie