Re: AIO v2.5

Hi,

On 2025-03-25 17:19:15 -0700, Noah Misch wrote:
> On Mon, Mar 24, 2025 at 09:18:06PM -0400, Andres Freund wrote:
> > @@ -296,7 +299,9 @@ pgaio_io_call_complete_local(PgAioHandle *ioh)
> >  
> >  	/*
> >  	 * Note that we don't save the result in ioh->distilled_result, the local
> > -	 * callback's result should not ever matter to other waiters.
> > +	 * callback's result should not ever matter to other waiters. However, the
> > +	 * local backend does care, so we return the result as modified by local
> > +	 * callbacks, which then can be passed to ioh->report_return->result.
> >  	 */
> >  	pgaio_debug_io(DEBUG3, ioh,
> >  				   "after local completion: distilled result: (status %s, id %u, error_data %d, result %d), raw_result: %d",
> 
> Should this debug message remove the word "distilled", since this commit
> solidifies distilled_result as referring to the complete_shared result?

Good point, updated.


> > Subject: [PATCH v2.12 01/28] aio: Be more paranoid about interrupts
> Ready for commit
> > Subject: [PATCH v2.12 02/28] aio: Pass result of local callbacks to
> >  ->report_return
> 
> Ready for commit w/ up to one cosmetic change:
> 

And pushed. Together with the s/pgaio_io_prep_/s/pgaio_io_start_/ renaming
we've been discussing. Btw, I figured out the origin of that, I was just
mirroring the liburing API...

Thanks again for the reviews.


> > Subject: [PATCH v2.12 03/28] aio: Add liburing dependency
> 
> Ready for commit
> 
> 
> > Subject: [PATCH v2.12 04/28] aio: Add io_method=io_uring
> 
> Ready for commit w/ open_fd.fixup

Yay.  Planning to push those soon.


> > Subject: [PATCH v2.12 05/28] aio: Implement support for reads in smgr/md/fd
> 
> Ready for commit w/ up to two cosmetic changes:

Cool.


> > +/*
> > + * AIO error reporting callback for mdstartreadv().
> > + *
> > + * Errors are encoded as follows:
> > + * - PgAioResult.error_data != 0 encodes IO that failed with errno != 0
> 
> I recommend replacing "errno != 0" with either "that errno" or "errno ==
> error_data".

Applied.


> Second, the aio_internal.h comment changes discussed in
> postgr.es/m/20250325155808.f7.nmisch@google.com and earlier.

Here's my current version of that:

 * Note that the externally visible functions to start IO
 * (e.g. FileStartReadV(), via pgaio_io_start_readv()) move an IO from
 * PGAIO_HS_HANDED_OUT to at least PGAIO_HS_STAGED and at most
 * PGAIO_HS_COMPLETED_LOCAL (at which point the handle will be reused).

Does that work?

I think I'll push that as part of the comment updates patch instead of
"Implement support for reads in smgr/md/fd", unless you see a reason to do so
differently. I'd have done it in the patch to s/prep/start/, but then it would
reference functions that don't exist yet...


> > Subject: [PATCH v2.12 06/28] aio: Add README.md explaining higher level design
> 
> Ready for commit

Cool.

Comments in it reference PGAIO_HCB_SHARED_BUFFER_READV, so I'm inclined to
reorder it until after "bufmgr: Implement AIO read support".

There's also a small change in a new patch in the series (not yet sent), due
to the changes related to emitting WARNINGs about checksum failures to the
client connection.  I think that part is fine, but...


> (This and the previous patch have three spots that would change with the
> s/prep/start/ renames.  No opinion on whether to rename before or rename
> after.)

I thought it'd be better to do the renaming first.



> > Subject: [PATCH v2.12 07/28] localbuf: Track pincount in BufferDesc as well
> 
> The plan here looks good:
> postgr.es/m/dbeeaize47y7esifdrinpa2l7cqqb67k72exvuf3appyxywjnc@7bt76mozhcy2

> > Subject: [PATCH v2.12 08/28] bufmgr: Implement AIO read support
> 
> See review here and later discussion:
> postgr.es/m/20250325022037.91.nmisch@google.com

I'm working on a version with those addressed.


> > Subject: [PATCH v2.12 09/28] bufmgr: Use AIO in StartReadBuffers()
> 
> Ready for commit after a batch of small things, all but one of which have no
> implications beyond code cosmetics.

Yay.


> I like the test coverage (by the end of the patch series).

I'm really shocked just how bad our test coverage for a lot of this is today
:(


> For anyone else following, I found "diff -w" helpful for the bufmgr.c
> changes.  That's because a key part is former WaitReadBuffers() code moving
> up an indentation level to its home in new subroutine AsyncReadBuffers().

For reviewing changes that move stuff around a lot I find this rather helpful:
git diff --color-moved --color-moved-ws=ignore-space-change

That highlights removed code differently from moved code, and due to
ignore-space-change considers code that changed just due to space changes, to
be moved.


> >  	Assert(*nblocks == 1 || allow_forwarding);
> >  	Assert(*nblocks > 0);
> >  	Assert(*nblocks <= MAX_IO_COMBINE_LIMIT);
> > +	Assert(*nblocks == 1 || allow_forwarding);
> 
> Duplicates the assert three lines back.

Ah, it was moved into ce1a75c4fea, which I didn't notice while rebasing...


> > +		nblocks = aio_ret->result.result;
> > +
> > +		elog(DEBUG3, "partial read, will retry");
> > +
> > +	}
> > +	else if (aio_ret->result.status == PGAIO_RS_ERROR)
> > +	{
> > +		pgaio_result_report(aio_ret->result, &aio_ret->target_data, ERROR);
> > +		nblocks = 0;			/* silence compiler */
> > +	}
> >  
> >  	Assert(nblocks > 0);
> >  	Assert(nblocks <= MAX_IO_COMBINE_LIMIT);
> >  
> > +	operation->nblocks_done += nblocks;
> 
> I struggled somewhat from the variety of "nblocks" variables: this local
> nblocks, operation->nblocks, actual_nblocks, and *nblocks in/out parameters of
> some functions.  No one of them is clearly wrong to use the name, and some of
> these names are preexisting.  That said, if you see opportunities to push in
> the direction of more-specific names, I'd welcome it.
> 
> For example, this local variable could become add_to_nblocks_done instead.

I named it "newly_read_blocks", hope that works?


> > +		AsyncReadBuffers(operation, &nblocks);
> 
> I suggest renaming s/nblocks/ignored_nblocks_progress/ here.

Adopted.


Unfortunately I didn't see a good way to reduce the amount of the other
nblocks variables, as they are all, I think, preexisting.


> > +	 * If we need to wait for IO before we can get a handle, submit already
> > +	 * staged IO first, so that other backends don't need to wait. There
> 
> s/already staged/already-staged/.  Normally I'd skip this as nitpicking, but I
> misread this particular sentence twice, as "submit" being the subject that
> "staged" something.  (It's still nitpicking, alas.)

Makes sense - it doesn't help that it was at a linebreak...


> >  		/*
> >  		 * How many neighboring-on-disk blocks can we scatter-read into other
> >  		 * buffers at the same time?  In this case we don't wait if we see an
> > -		 * I/O already in progress.  We already hold BM_IO_IN_PROGRESS for the
> > +		 * I/O already in progress.  We already set BM_IO_IN_PROGRESS for the
> >  		 * head block, so we should get on with that I/O as soon as possible.
> > -		 * We'll come back to this block again, above.
> > +		 *
> > +		 * We'll come back to this block in the next call to
> > +		 * StartReadBuffers() -> AsyncReadBuffers().
> 
> Did this mean to say "WaitReadBuffers() -> AsyncReadBuffers()"?  I'm guessing
> so, since WaitReadBuffers() is the one that loops.  It might be referring to
> read_stream_start_pending_read()'s next StartReadBuffers(), though.

I was referring to the latter, as that is the more common case (it's pretty
easy to hit if you e.g. have multiple sequential scans on the same table
going).


> I think this could just delete the last sentence.  The function header comment
> already mentions the possibility of reading a subset of the request.  This
> spot doesn't need to detail how the higher layers come back to here.

Agreed.


> > +		smgrstartreadv(ioh, operation->smgr, forknum,
> > +					   blocknum + nblocks_done,
> > +					   io_pages, io_buffers_len);
> > +		pgstat_count_io_op_time(io_object, io_context, IOOP_READ,
> > +								io_start, 1, *nblocks_progress * BLCKSZ);
> 
> We don't assign *nblocks_progress until lower in the function, so I think
> "io_buffers_len" should replace "*nblocks_progress" here.  (This is my only
> non-cosmetic comment on this patch.)

Good catch!



> > Subject: [PATCH v2.12 16/28] aio: Add test_aio module
> 
> > +use List::Util qw(sample);
> 
> sample() is new in 2020:
> https://metacpan.org/release/PEVANS/Scalar-List-Utils-1.68/source/Changes#L100
> 
> Hence, I'd expect some buildfarm failures.  I'd try to use shuffle(), then
> take the first N elements.

Hah. Bilal's patch was using shuffe(). I wanted to reduce the number of
iterations and first did as you suggest and then saw that there's a nicer
way...

Done that way again...


> > +++ b/src/test/modules/test_aio/test_aio.c
> > @@ -0,0 +1,712 @@
> > +/*-------------------------------------------------------------------------
> > + *
> > + * delay_execution.c
> > + *		Test module to allow delay between parsing and execution of a query.
> > + *
> > + * The delay is implemented by taking and immediately releasing a specified
> > + * advisory lock.  If another process has previously taken that lock, the
> > + * current process will be blocked until the lock is released; otherwise,
> > + * there's no effect.  This allows an isolationtester script to reliably
> > + * test behaviors where some specified action happens in another backend
> > + * between parsing and execution of any desired query.
> > + *
> > + * Copyright (c) 2020-2025, PostgreSQL Global Development Group
> > + *
> > + * IDENTIFICATION
> > + *	  src/test/modules/test_aio/test_aio.c
> 
> To elaborate on my last review, the entire header comment was a copy from
> delay_execution.c.  v2.12 fixes the IDENTIFICATION, but the rest needs
> updates.

I was really too tired that day... Embarassing.

Greetings,

Andres Freund