Re: AIO v2.5

On Tue, Mar 18, 2025 at 4:12 PM Andres Freund <andres@anarazel.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