From: Antonin Houska <ah@cybertec.at>
To: Andres Freund <andres@anarazel.de>
cc: Noah Misch <noah@leadboat.com>, pgsql-hackers@postgresql.org,
    Thomas Munro <thomas.munro@gmail.com>,
    Heikki Linnakangas <hlinnaka@iki.fi>,
    Robert Haas <robertmhaas@gmail.com>,
    Jakub Wartak <jakub.wartak@enterprisedb.com>,
    Jelte Fennema-Nio <postgres@jeltef.nl>
Subject: Re: AIO v2.5
In-reply-to: <4b3f32ug3cayekysqlgspz2qjmeb7lca3gvazayglxr2m3d4dv@il33accgsji7>
References: <uvrtrknj4kdytuboidbhwclo4gxhswwcpgadptsjvjqcluzmah@brqs62irg4dt>
 <clt7rl56kxjcnjtqd7fsajkst232c3yh57ggtmppwp5hmtl4os@i3iibeftfrsp>
 <vz5i2x2wkjjpp6l2z4g4l3umpxktrxsejgdwefi7n6kzw666h3@rnhlvlpadig7>
 <ge6nsuddurhpmll3xj22vucvqwp4agqz6ndtcf2mhyeydzarst@l75dman5x53p>
 <20250311194108.c5.nmisch@google.com>
 <5dzyoduxlvfg55oqtjyjehez5uoq6hnwgzor4kkybkfdgkj7ag@rbi4gsmzaczk>
 <20250312035743.f5.nmisch@google.com>
 <w6uiicyou7hzq47mbyejubtcyb2rngkkf45fk4q7inue5kfbeo@bbfad3qyubvs>
 <4b3f32ug3cayekysqlgspz2qjmeb7lca3gvazayglxr2m3d4dv@il33accgsji7>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
   message dated "Wed, 12 Mar 2025 19:23:47 -0400."
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Date: Thu, 13 Mar 2025 11:53:03 +0100
Message-ID: <17906.1741863183@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/17906.1741863183%40localhost>
Precedence: bulk

--=-=-=
Content-Type: text/plain

Andres Freund <andres@anarazel.de> wrote:

> Attached is v2.7, with the following changes:

Attached are a few proposals for minor comment fixes.

Besides that, it occurred to me when I was trying to get familiar with the
patch set (respectable work, btw) that an additional Assert() statement could
make sense:

diff --git a/src/backend/storage/aio/aio.c b/src/backend/storage/aio/aio.c
index a9c351eb0dc..325688f0f23 100644
--- a/src/backend/storage/aio/aio.c
+++ b/src/backend/storage/aio/aio.c
@@ -413,6 +413,7 @@ pgaio_io_stage(PgAioHandle *ioh, PgAioOp op)
        bool            needs_synchronous;
 
        Assert(ioh->state == PGAIO_HS_HANDED_OUT);
+       Assert(pgaio_my_backend->handed_out_io == ioh);
        Assert(pgaio_io_has_target(ioh));
 
        ioh->op = op;

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment; filename=aio_comments.diff

diff --git a/src/backend/storage/aio/aio_io.c b/src/backend/storage/aio/aio_io.c
index 89376ff4040..7879e556263 100644
--- a/src/backend/storage/aio/aio_io.c
+++ b/src/backend/storage/aio/aio_io.c
@@ -107,7 +107,7 @@ pgaio_io_prep_writev(PgAioHandle *ioh,
 
 /*
  * Execute IO operation synchronously. This is implemented here, not in
- * method_sync.c, because other IO methods lso might use it / fall back to it.
+ * method_sync.c, because other IO methods also might use it / fall back to it.
  */
 void
 pgaio_io_perform_synchronously(PgAioHandle *ioh)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 9483596e63a..df9357c78c0 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -7329,7 +7329,7 @@ local_buffer_writev_stage(PgAioHandle *ioh, uint8 cb_data)
 }
 
 
-/* readv callback is is passed READ_BUFFERS_* flags as callback data */
+/* readv callback is passed READ_BUFFERS_* flags as callback data */
 const PgAioHandleCallbacks aio_shared_buffer_readv_cb = {
 	.stage = shared_buffer_readv_stage,
 	.complete_shared = shared_buffer_readv_complete,
diff --git a/src/include/storage/aio_internal.h b/src/include/storage/aio_internal.h
index ca6cbde11a6..cf4eb102adc 100644
--- a/src/include/storage/aio_internal.h
+++ b/src/include/storage/aio_internal.h
@@ -205,9 +205,9 @@ typedef struct PgAioBackend
 	dclist_head idle_ios;
 
 	/*
-	 * Only one IO may be returned by pgaio_io_acquire()/pgaio_io_acquire()
+	 * Only one IO may be returned by pgaio_io_acquire()/pgaio_io_acquire_nb()
 	 * without having been either defined (by actually associating it with IO)
-	 * or by released (with pgaio_io_release()). This restriction is necessary
+	 * or released (with pgaio_io_release()). This restriction is necessary
 	 * to guarantee that we always can acquire an IO. ->handed_out_io is used
 	 * to enforce that rule.
 	 */

--=-=-=--