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>
In-Reply-To: 
 <CAAKRu_ZM1epxTdt2=4-g4ff6UC09zne+0xg_gNv3d7LEcxEvNA@mail.gmail.com>
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: Fri, 6 Mar 2026 16:18:13 +0300
Message-ID: 
 <CAN55FZ0ymY=XfaHQXHKG+Mbit6BZzZ9d-UWb6dmRXYm_xCvfQg@mail.gmail.com>
Subject: Re: Don't synchronously wait for already-in-progress IO in read
 stream
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>, Andres Freund <andres@anarazel.de>,
	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/CAN55FZ0ymY%3DXfaHQXHKG%2BMbit6BZzZ9d-UWb6dmRXYm_xCvfQg%40mail.gmail.com>
Precedence: bulk

Hi,

On Tue, 3 Mar 2026 at 22:47, Melanie Plageman <melanieplageman@gmail.com> w=
rote:
>
> On Thu, Feb 5, 2026 at 11:56=E2=80=AFAM Nazir Bilal Yavuz <byavuz81@gmail=
.com> wrote:
> >
>
> I did finally review Andres' test patches and have included my review
> feedback here as well.
>
> "aio: Refactor tests in preparation for more tests" (v4-0001) looks
> good to me as well. I included one tiny idea AI suggested to me in a
> follow-on patch (v4-0002).

This makes sense.

> > diff --git a/src/test/modules/test_aio/t/004_read_stream.pl
> > b/src/test/modules/test_aio/t/004_read_stream.pl
> > +foreach my $method (TestAio::supported_io_methods())
> > +{
> > +    $node->adjust_conf('postgresql.conf', 'io_method', 'worker');
> > +    $node->start();
> > +    test_io_method($method, $node);
> > +    $node->stop();
> > +}
> >
> > This seems wrong, we always test io_method=3Dworker. I think we need to
> > replace 'worker' with the $method. Also, we can add check below to the
> > test_io_method function in the 004_read_stream.pl:
> > ```
> >     is($node->safe_psql('postgres', 'SHOW io_method'),
> >         $io_method, "$io_method: io_method set correctly");
>
> Good catch. Fixed. I also found a few other small things in this patch
> (v4-0003) which I put in v4-0004.

These look good.

> Some ideas I had that I didn't include in v4-0003 because its Andres
> patch and is subjective:
>
> For test_repeated_blocks, the first test:
>
>     # test miss of the same block twice in a row
>     $psql->query_safe(
>         qq/
> SELECT evict_rel('largeish');
> /);
>     $psql->query_safe(
>         qq/
> SELECT * FROM read_stream_for_blocks('largeish', ARRAY[0, 2, 2, 4, 4]);
> /);
>     ok(1, "$io_method: stream missing the same block repeatedly");
>
> It says that it will miss the same block repeatedly, is that because
> we won't start a read for any of the blocks until after
> read_stream_get_block has returned all of them? If so, could be
> clearer in the comment. Not everyone understands all the read stream
> internals.

I think we start a read of blocks because we hit stream->distance but
it doesn't affect any consecutive same block numbers. What I
understood is:

Since io_combine_limit is 1, there won't be any IO combining.

0th block (0), miss, distance is 1; StartReadBuffersImpl() and
WaitReadBuffers() are called for 0th block.
1th block (2), miss, distance is 2, StartReadBuffersImpl() is called.
2th block (2), miss, distance is 2, StartReadBuffersImpl() and
WaitReadBuffers() are called 1th block.
3th block (4), miss, distance is 4, StartReadBuffersImpl() is called.
4th block (4), miss, distance is 4, StartReadBuffersImpl() and
WaitReadBuffers() are called 2, 3 and 4th blocks.

> I know a lot of other tests do this, but I find it so hard to read the
> test with the SQL is totally left-aligned like that -- especially with
> comments interspersed. You can easily flow the queries on multiple
> lines and indent it more.

I agree with you.

> For test_repeated_blocks, the second test:
>
>     # test hit of the same block twice in a row
>     $psql->query_safe(
>         qq/
> SELECT evict_rel('largeish');
> /);
>     $psql->query_safe(
>         qq/
> SELECT * FROM read_stream_for_blocks('largeish', ARRAY[0, 1, 2, 3, 4,
> 5, 6, 5, 4, 3, 2, 1, 0]);
> /);
>     ok(1, "$io_method: stream accessing same block");
>
> I assume that the second access of 2 is a hit because we actually did
> IO for the first one (unlike in the earlier case)?

I think so but to clarify, all second access of [2, 1, 0] blocks are hit; r=
ight?

> For test_inject_foreign:
>
> In general, I am not ramped up enough on injection point stuff to know
> if the actual new injection point stuff you added in test_aio.c is is
> correct and optimal, but I did review the read stream additions to
> test_aio.c and the tests added to 004_read_stream.pl.
>
> For test_inject_foreign, the 3rd test:
>
>     # Test read stream encountering two buffers that are undergoing the s=
ame
>     # IO, started by another backend
>
> I see that psql_b is requesting 3 blocks which can be combined into 1
> IO, which makes it different than the 1st foreign IO test case:
>
>     ###
>     # Test read stream encountering buffers undergoing IO in another back=
end,
>     # with the other backend's reads succeeding.
>     ###
>
> where psql_b only requests 1 but I don't really see how these are
> covering different code. Maybe if the read stream one (psql_a) is
> doing a combined IO it might exercise slightly different code, but
> otherwise I don't get it.

I think the main difference is that:

>     ###
>     # Test read stream encountering buffers undergoing IO in another back=
end,
>     # with the other backend's reads succeeding.
>     ###

SELECT array_agg(blocknum) FROM read_stream_for_blocks('largeish',
ARRAY[0, 2, 5, 7]);

We need to join waiting block number 5 and then start another IO for
block number 7.

>     # Test read stream encountering two buffers that are undergoing the s=
ame
>     # IO, started by another backend

SELECT array_agg(blocknum) FROM read_stream_for_blocks('largeish',
ARRAY[0, 2, 4]);

We need to join waiting block number 2 but after waiting for an IO, IO
for block number 4 should be already completed too. We don't need to
start IO like the other case.

--=20
Regards,
Nazir Bilal Yavuz
Microsoft