Re: AIO v2.5

On Tue, Mar 18, 2025 at 4:12 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Attached is v2.10,

I noticed a few comments could be improved in 0011: bufmgr: Use AIO
in StartReadBuffers()

In WaitReadBuffers(), this comment is incomplete:

/*
- * Skip this block if someone else has already completed it. If an
- * I/O is already in progress in another backend, this will wait for
- * the outcome: either done, or something went wrong and we will
- * retry.
+ * If there is an IO associated with the operation, we may need to
+ * wait for it. It's possible for there to be no IO if
*/

In WaitReadBuffers(), too many thes

/*
* Most of the the the one IO we started will read in everything. But
* we need to deal with short reads and buffers not needing IO
* anymore.
*/

In ReadBuffersCanStartIO()

+ /*
+ * Unfortunately a false returned StartBufferIO() doesn't allow to
+ * distinguish between the buffer already being valid and IO already
+ * being in progress. Since IO already being in progress is quite
+ * rare, this approach seems fine.
+ */

maybe reword "a false returned StartBufferIO()"

Above and in AsyncReadBuffers()

* To support retries after short reads, the first operation->nblocks_done is
* buffers are skipped.

can't quite understand this

+ * On return *nblocks_progres is updated to reflect the number of buffers
progress spelled wrong

* A secondary benefit is that this would allows us to measure the time in
* pgaio_io_acquire() without causing undue timer overhead in the common,
* non-blocking, case. However, currently the pgstats infrastructure
* doesn't really allow that, as it a) asserts that an operation can't
* have time without operations b) doesn't have an API to report
* "accumulated" time.
*/

allows->allow

What would the time spent in pgaio_io_acquire() be reported as? Time
submitting IOs? Time waiting for a handle? And what is "accumulated"
time here? It seems like you just add the time to the running total
and that is already accumulated.

- Melanie