MIME-Version: 1.0
References: 
 <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>
 <CAAKRu_bxajARZ0sXg_P=ZN76f1GofBmojE6HbQd7chjjhzWnRg@mail.gmail.com>
 <tjhnl6hiooo4aj6tktyehf4pn4yxcaqndlqebgpmynw4tmticq@odfcssnjf2qu>
In-Reply-To: <tjhnl6hiooo4aj6tktyehf4pn4yxcaqndlqebgpmynw4tmticq@odfcssnjf2qu>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Thu, 26 Mar 2026 17:43:17 -0400
Message-ID: 
 <CAAKRu_a8htJqvgMAMtA54zX35EZKhx3j7nLDXhksbciCxiNaJQ@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_a8htJqvgMAMtA54zX35EZKhx3j7nLDXhksbciCxiNaJQ%40mail.gmail.com>
Precedence: bulk

On Wed, Mar 25, 2026 at 5:58=E2=80=AFPM Andres Freund <andres@anarazel.de> =
wrote:
>
> I'm planning to commit 0001 soon - it hasn't changed in a while. Then I'd=
 like
> to get 0002 committed soon after, but I'll hold off for that until tomorr=
ow,
> given that nobody has looked at it (as it's new).  I think 0004-0007 can =
be
> committed too, but I am not sure if you (Melanie) want to do so.

This is a review of 0002

in read_buffers():
- you forgot to initialize operation->forknum
- I think the output columns could really use a bit of documentation
- I assume that using uint16 for nblocks and nios is to make sure it
plays nice with the input nblocks which is an int32 -- it doesn't
matter for the range of values you're using, but it would be better if
you could just use uint32 everywhere but I guess because we only have
int4 in SQL you can't.
anyway, I find all the many different types of ints and uints in
read_buffers() pretty sus. Like why do you need this cast to int16?
that seems...wrong and unnecessary?
        values[3] =3D Int32GetDatum((int16) nblocks_this_io);

in the evict_rel() refactor:
- you need invalidate_one_block() to use the forknum parameter because
otherwise temp tables won't evict any forks except the main fork

for the tests themselves:
- for the first test,
        # check that one larger read is done as multiple reads
isn't the comment actually the opposite of what it is testing?

0|0|t|2 -- would be 1 2 block io starting at 0, no?

seems like something like
# check that consecutive misses are combined into one read
would be better

- for this comment:
# but if we do it again, i.e. it's in s_b, there will be two operations
technically you are also doing this for temp tables, so the comment
isn't entirely correct.

- For this test:
# Verify that we aren't doing reads larger than io_combine_limit
isn't this more just covering the logic in read_buffers()? AFAICT
StartReadBuffers() only worries about the max IOs it can combine if it
is near the segment boundary

- For this:
$psql_a->query_safe(qq|SELECT invalidate_rel_block('$table', 1)|);
$psql_a->query_safe(qq|SELECT invalidate_rel_block('$table', 2)|);
$psql_a->query_safe(qq|SELECT * FROM read_buffers('$table', 3, 2)|);
psql_like(
        $io_method,
        $psql_a,
        "$persistency: read buffers, miss 1-2, hit 3-4",
        qq|SELECT blockoff, blocknum, needs_io, nblocks FROM
read_buffers('$table', 1, 4)|,
        qr/^0\|1\|t\|2\n2\|3\|f\|1\n3\|4\|f\|1$/,
        qr/^$/);

I think this is a duplicate. There is one before and after the "verify
we aren't doing reads larger than io_combine_limit"

- It may be worth adding one more test case which is IO in progress on
the last block since you have in-progress as the first and the middle
blocks but not as the last block

# Test in-progress IO on the last block of the range
$psql_a->query_safe(qq|SELECT evict_rel('$table')|);
$psql_a->query_safe(
    qq|SELECT read_rel_block_ll('$table', 3, wait_complete=3D>false)|);
psql_like(
    $io_method,
    $psql_a,
    "$persistency: read buffers, in-progress 3, read 1-3",
    qq|SELECT blockoff, blocknum, needs_io, nblocks FROM
read_buffers('$table', 1, 3)|,
    qr/^0\|1\|t\|2\n2\|3\|f\|1$/,
    qr/^$/);

- Melanie