Re: Don't synchronously wait for already-in-progress IO in read stream

Hi,

On 2026-03-18 16:16:14 -0400, Andres Freund wrote:
> > I don't think we'll be able to easily test READ_BUFFER_ALREADY_DONE
> > (though perhaps we aren't testing it for shared buffers either?).
>
> We do reach the READ_BUFFER_ALREADY_DONE in PrepareHeadBufferReadIO(), but
> only due to io_method=sync peculiarities (as that only actually performs the
> IO later when waiting, it's easy to have two IOs for the same block).
>
>
> It's probably worth adding tests for that, although I suspect it should be in
> 001_aio.pl - no read stream required to hit it. I can give it a shot, if you
> want?

I started writing some tests and realized that these commits had made that
somewhat harder - by not using StartBufferIO() anymore, the existing test
infrastructure for StartBufferIO() (in test_aio) became less useful. In
effect it actually reduced coverage some.

Thinking about it more, I also got worried about the duplicating of
logic. It's perhaps acceptable with the patches as-is, but we'll soon need
something very similar for AIO writes. Then we'd end up with like 5 variants,
because we'd still need the existing StartBufferIO() for some cases where we
do want to wait (e.g. the edge case in ExtendBufferedRelShared()).

In the attached prototype I replaced your patch introducing
PrepareHeadBufferReadIO()/ PrepareAdditionalBufferReadIO() with one that
instead revises StartBufferIO() to have the enum return value you introduced
and a PgAioWaitRef * argument that callers that would like to asynchronously
wait for the IO to complete (others pass NULL). There are some other cleanups
in it too, see the commit message for more details.

Your final patch doesn't change a whole lot due to this, instead of calling
PrepareHeadBufferReadIO() it now calls StartBufferIO(wait=true,
&operation->io_wref) and instead of PrepareAdditionalBufferReadIO() it does
StartBufferIO(wait=false, NULL). That means it has to set
operation->foreign_io to true itself, but that seems not problematic.

As part of this I replaced the following comment:
+ /*
+ * Submit any staged IO before checking for in-progress IO. Without this,
+ * the wref check below could find IO that this backend staged but hasn't
+ * submitted yet. Waiting on that would PANIC because the owner can't wait
+ * on its own staged IO.
+ */
+ pgaio_submit_staged();

as I am pretty sure that can't be reached. I added an assert + explanation.

I also updated "Restructure AsyncReadBuffers()" to move
pgstat_prepare_report_checksum_failure() and the computation of flags to
before the ReadBuffersCanStartIO(). And added a comment explaining why little
should be added between the ReadBuffersCanStartIO() calls.

Thoughts?

I still want to expand the tests a bit, but I thought that we should resolve
this structural issue first.

Greetings,

Andres Freund