MIME-Version: 1.0
References: <f3xxfrkafjxpyqxywcxricxgyizjirfceychyxsgn7bwjp5eda@kwbduhy7tfmu>
In-Reply-To: <f3xxfrkafjxpyqxywcxricxgyizjirfceychyxsgn7bwjp5eda@kwbduhy7tfmu>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Tue, 31 Mar 2026 15:31:39 -0400
Message-ID: 
 <CAAKRu_YvSjYvKtsTsRqm0=Jy0QhpUTsccRd66BsSKrdN-7ZA1g@mail.gmail.com>
Subject: Re: AIO / read stream heuristics adjustments for index prefetching
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>,
	Peter Geoghegan <pg@bowt.ie>, Tomas Vondra <tv@fuzzy.cz>,
 Nazir Bilal Yavuz <byavuz81@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAAKRu_YvSjYvKtsTsRqm0%3DJy0QhpUTsccRd66BsSKrdN-7ZA1g%40mail.gmail.com>
Precedence: bulk

On Tue, Mar 31, 2026 at 12:02=E2=80=AFPM Andres Freund <andres@anarazel.de>=
 wrote:
>
> 0001+0002:  Return whether WaitReadBuffers() needed to wait
>
>     The first patch allows pgaio_wref_check_done() to work more reliably =
with
>     io_uring. Until now it only was able to return true if userspace alre=
ady
>     had consumed the kernel's completion event, but returned false otherw=
ise.
>     That's not really incorrect, just suboptimal.
>
>     The second patch returns whether WaitReadBuffers() needed to wait for
>     IO. This is useful for a) instrumentation like in [2] and b) to provi=
de
>     information to the read_stream heuristics to control how aggressive t=
o
>     perform read ahead.

These both look good to me except in 0001 you left an XXX in
pgaio_wref_check_done() that I think is the very thing that commit
does.

> 0003:  read_stream: Issue IO synchronously while in fast path

LGTM

> 0004:  read_stream: Prevent distance from decaying too quickly
>
>     There are two minor questions here:
>     - Should read_stream_pause()/read_stream_resume() restore the "holdof=
f"
>       counter?  I doubt it matters for the prospective user, since it wil=
l
>       only be used when the lookahead distance is very large.

I don't really understand this. We have to do this with distance
because we set it to 0 and use distance =3D=3D 0 to indicate stream end.
read_stream_pause() doesn't set the distance_decay_holoff to 0. If you
mean, should we reset holdoff to its initial value, then I don't think
so. I imagine that users doing a lot of pause and resume may not have
high distance.

>     - For how long to hold off distance reductions?  Initially I was torn
>       between using "max_pinned_buffers" (Min(max_ios * io_combine_limit,
>       cap)) and "max_ios" ([maintenance_]effective_io_concurrency). But I
>       think the former makes more sense, as we otherwise won't allow for =
far
>       enough readahead when doing IO combining, and it does seem to make =
sense
>       to hold off decay for long enough that the maximum lookahead could =
not
>       theoretically allow us to start an IO.

I agree. 0004 LGTM otherwise.

- Melanie