MIME-Version: 1.0
References: 
 <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>
 <CAAKRu_YiaJgbmHGTFHRJEP9+AtVCt+gHTd1rmXm05agT_1Mx2A@mail.gmail.com>
 <rk56uxuqxi3udzu5oqc7qft5u7nncau5d2avlbui6obd7ybols@wlu2euvawj7t>
In-Reply-To: <rk56uxuqxi3udzu5oqc7qft5u7nncau5d2avlbui6obd7ybols@wlu2euvawj7t>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Wed, 25 Mar 2026 11:33:08 -0400
Message-ID: 
 <CAAKRu_bxajARZ0sXg_P=ZN76f1GofBmojE6HbQd7chjjhzWnRg@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_bxajARZ0sXg_P%3DZN76f1GofBmojE6HbQd7chjjhzWnRg%40mail.gmail.com>
Precedence: bulk

On Wed, Mar 25, 2026 at 11:15=E2=80=AFAM Andres Freund <andres@anarazel.de>=
 wrote:
>
> > > In the attached prototype I replaced your patch introducing
> > > PrepareHeadBufferReadIO()/ PrepareAdditionalBufferReadIO() with one t=
hat
> > > instead revises StartBufferIO() to have the enum return value you int=
roduced
> > > and a PgAioWaitRef * argument that callers that would like to asynchr=
onously
> > > wait for the IO to complete (others pass NULL). There are some other =
cleanups
> > > 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.
>
> Do you have an alternative suggestion? We could add a dedicated parameter=
 for
> that, but then it just opens up different ways of calling the function wi=
th
> the wrong arguments.

Yea, I don't know. The cleanest way to handle callers with different
intentions and tolerances is to have different functions that allow
different behavior. But that's the opposite of what we're currently
trying to do :) I tried to come up with some intention-related enum
input argument, but that seems like a bit of over-engineering.

> > 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=
=94before
> > detailing argument combinations=E2=80=94why a caller would want to pass
> > io_wref might help.
>
> I'm not entirely sure what you'd like here.  Would the following comment
> do the trick?
>
>  * In several scenarios the buffer may already be undergoing I/O in this =
or
>  * another backend. How to best handle that depends on the caller's
>  * situation. It might be appropriate to wait synchronously (e.g., becaus=
e the
>  * buffer is about to be invalidated); wait asynchronously, using the buf=
fer's
>  * IO wait reference (e.g., because the caller is doing readahead and doe=
sn't
>  * need the buffer to be ready immediately); or to not wait at all (e.g.,
>  * because the caller is trying to combine IO for this buffer with anothe=
r
>  * buffer).
>  *
>  * How and whether to wait is controlled by the wait in io_wref parameter=
s. In
>  * detail:
>  *
>  * <existing comment>

Sounds good to me.

> > 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_wr=
ef
> > 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 NUL=
L
> > -- because they don't want to synchronously wait.
>
> Hm. I started out proposing that we should just add an assert documenting=
 this
> is a nonsensical combination. But when writing the comment for that I rea=
lized
> that it theoretically could make sense to pass in wait =3D=3D false and i=
o_wref !=3D
> NULL, if you wanted to get a wait reference, but would not want to do a
> WaitIO() if there's no wait reference set.
>
> I don't think that's something we need right now, but ...

We should somehow express this in the comment.

- Melanie