Feedback-ID: id4a34324:Fastmail
Date: Fri, 27 Mar 2026 17:37:39 -0400
From: Andres Freund <andres@anarazel.de>
To: Melanie Plageman <melanieplageman@gmail.com>
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>
Subject: Re: Don't synchronously wait for already-in-progress IO in read
 stream
Message-ID: <r74jwetiwnwzuubnd2srshy3ks7flqviesxh4tgf4r74oj7zum@zms5nncdazxg>
References: <prkj4fv7dvyew4lbad2g5l346nn6u66inndwx7nzzap5c3bdmh@fds54c2hx3yd>
 <42rdu4q44kvsq53fz5qgzuawqpaytvnemsnquynlfch5mqhc2m@6ytnlgivtzro>
 <CAAKRu_YiaJgbmHGTFHRJEP9+AtVCt+gHTd1rmXm05agT_1Mx2A@mail.gmail.com>
 <rk56uxuqxi3udzu5oqc7qft5u7nncau5d2avlbui6obd7ybols@wlu2euvawj7t>
 <CAAKRu_bxajARZ0sXg_P=ZN76f1GofBmojE6HbQd7chjjhzWnRg@mail.gmail.com>
 <tjhnl6hiooo4aj6tktyehf4pn4yxcaqndlqebgpmynw4tmticq@odfcssnjf2qu>
 <CAAKRu_a8htJqvgMAMtA54zX35EZKhx3j7nLDXhksbciCxiNaJQ@mail.gmail.com>
 <wmqrglrv4eq2m2kzgbjbtmmuctklg7q4ylwo22rks7r7mlbf3w@dn6txz6znmtr>
 <us6m5jdjxmveyan77tdg2tysjls4yu3tsohpvjjedz6pnjkyn7@2g6b2asvgug3>
 <CAAKRu_ZEddj8O10z7G2NfOG8bB+Qk+1V_Xt2YpnVS3bVJrjHLg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAAKRu_ZEddj8O10z7G2NfOG8bB+Qk+1V_Xt2YpnVS3bVJrjHLg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/r74jwetiwnwzuubnd2srshy3ks7flqviesxh4tgf4r74oj7zum%40zms5nncdazxg>
Precedence: bulk

Hi,

On 2026-03-27 17:17:25 -0400, Melanie Plageman wrote:
> On Fri, Mar 27, 2026 at 1:29 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2026-03-26 20:12:30 -0400, Andres Freund wrote:
> > > One test used did_io=(t|f). That was actually only needed once "aio: Don't
> > > wait for already in-progress IO" is in, as we might join the foreign IO. I
> > > chose to hide that by making that part of the query "did_io and not
> > > foreign_io", so we would detect if we were to falsely start IO ourselves.
> >
> > I ended up not liking did_io, as that seems misleading when we just needed to
> > wait for a foreign IO.  I instead named it io_reqd.
> 
> 0001 looks good to me except I don't get why you are still passing
> MAIN_FORKNUM to PrefetchBuffer() in invalidate_one_block()

Because I am stupid.


> In 0002, the test cases look good to me. I haven't gained more
> knowledge about injection point related code since my last review, so
> still no comment there (inj_io_completion_hook(), etc).
> 
> I didn't see anything amiss reviewing by eye. Running it through AI,
> it suggested that you should clear stdout between test cases in
> test_inject_foreign. I think this seems most relevant because in two
> back-to-back tests you are looking for the same output pattern.

Yea, that's a good call.

I'll give the BF a bit more time to digest f39cb8c0110 and then will push
0001/0002.


> It also pointed out that there is a pre-existing bug in
> inj_io_short_read_hook() where you pass the wrong parameter to the log
> message.
> 
> ereport(LOG, errmsg("short read injection point called, is enabled: %d",
>                 inj_io_error_state->enabled_reopen),
>                 errhidestmt(true), errhidecontext(true));
> 
> should be
> 
> ereport(LOG, errmsg("short read injection point called, is enabled: %d",
>                 inj_io_error_state->enabled_short_read),
>                 errhidestmt(true), errhidecontext(true));

I'll fix this as part of 0002 which touches related code, an injection point
debug message fixup doesn't seem to deserve its own commit message.

Greetings,

Andres Freund