Re: Don't synchronously wait for already-in-progress IO in read stream - Mailing list pgsql-hackers

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()

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