public inbox for [email protected]
help / color / mirror / Atom feedFrom: Melanie Plageman <[email protected]>
To: Andres Freund <[email protected]>
Cc: Nazir Bilal Yavuz <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: [email protected]
Cc: Peter Geoghegan <[email protected]>
Cc: Tomas Vondra <[email protected]>
Subject: Re: Don't synchronously wait for already-in-progress IO in read stream
Date: Fri, 27 Mar 2026 17:17:25 -0400
Message-ID: <CAAKRu_ZEddj8O10z7G2NfOG8bB+Qk+1V_Xt2YpnVS3bVJrjHLg@mail.gmail.com> (raw)
In-Reply-To: <us6m5jdjxmveyan77tdg2tysjls4yu3tsohpvjjedz6pnjkyn7@2g6b2asvgug3>
References: <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>
<CAAKRu_a8htJqvgMAMtA54zX35EZKhx3j7nLDXhksbciCxiNaJQ@mail.gmail.com>
<wmqrglrv4eq2m2kzgbjbtmmuctklg7q4ylwo22rks7r7mlbf3w@dn6txz6znmtr>
<us6m5jdjxmveyan77tdg2tysjls4yu3tsohpvjjedz6pnjkyn7@2g6b2asvgug3>
On Fri, Mar 27, 2026 at 1:29 PM Andres Freund <[email protected]> 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()
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.
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));
0003 LGTM.
I am still in the process of reviewing 0004.
- Melanie
view thread (31+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Don't synchronously wait for already-in-progress IO in read stream
In-Reply-To: <CAAKRu_ZEddj8O10z7G2NfOG8bB+Qk+1V_Xt2YpnVS3bVJrjHLg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox