Allow IO time to be counted without a matching IO operation in pg_stat_io

Since 999dec9ec6a816680, pg_stat_io can show read time with zero reads
for an IO Context: a foreign IO is counted as a read only in the
initiating backend, while other waiters record only the wait time. That
violates pgstat_bktype_io_stats_valid(). Relax the check to allow time
without a matching operation count, since we want to count read wait
time even in backends that did not initiate the read. This also enables
future accounting of waits on IO resources (e.g., AIO handles) in
backends that didn't start the IO.

Author: Andrey Rachitskiy <pl0h0yp1@gmail.com>
Reported-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Melanie Plageman <melanieplageman@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
Discussion: https://postgr.es/m/ak5lccE4qiQpOBHn@pryzbyj2023
Backpatch-through: 19

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index f1422826d2960ed0e9c2397dc1108a47361187a5..099e9b6f4e9ad66ede3870c1fffe7ebafe222269 100644 (file)
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3205,7 +3205,9 @@ description | Waiting for a newly initialized WAL file to reach durable storage
         <varname>object</varname> is not <literal>wal</literal>,
         or if <xref linkend="guc-track-wal-io-timing"/> is enabled
         and <varname>object</varname> is <literal>wal</literal>,
-        otherwise zero)
+        otherwise zero). This may include time spent waiting for a
+        read started by another backend. In that case
+        <structfield>reads</structfield> may still be zero.
        </para>
       </entry>
      </row>

diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 3908529872a319d00a315decc72a5c3b1dd34b88..169829eb020702062b405c350431cca5dc83f47d 100644 (file)
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1829,8 +1829,9 @@ WaitReadBuffers(ReadBuffersOperation *operation)
                                needed_wait = true;
 
                                /*
-                                * The IO operation itself was already counted earlier, in
-                                * AsyncReadBuffers(), this just accounts for the wait time.
+                                * This just accounts for the wait time. The IO operation
+                                * itself was already counted earlier in AsyncReadBuffers() --
+                                * either by us or by another backend if this is a foreign IO.
                                 */
                                pgstat_count_io_op_time(io_object, io_context, IOOP_READ,
                                                                                io_start, 0, 0);
@@ -2018,8 +2019,7 @@ AsyncReadBuffers(ReadBuffersOperation *operation, int *nblocks_progress)
         * A secondary benefit is that this would allow 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
+        * doesn't really allow that because it doesn't have an API to report
         * "accumulated" time.
         */
        ioh = pgaio_io_acquire_nb(CurrentResourceOwner, &operation->io_return);

diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index 4f7a39aaa0e8186235c817ef1ae7f54bde5e4fe1..8ec1aad5078fd479fb0b0f4e60dab3b1d379b2c3 100644 (file)
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -25,9 +25,10 @@ static bool have_iostats = false;
 
 /*
  * Check that stats have not been counted for any combination of IOObject,
- * IOContext, and IOOp which are not tracked for the passed-in BackendType. If
- * stats are tracked for this combination and IO times are non-zero, counts
- * should be non-zero.
+ * IOContext, and IOOp which are not tracked for the passed-in BackendType.
+ * Non-zero time with a zero operation count is allowed as there are cases
+ * where this may be appropriate -- like when a backend is waiting on IO
+ * initiated by another backend.
  *
  * The passed-in PgStat_BktypeIO must contain stats from the BackendType
  * specified by the second parameter. Caller is responsible for locking the
@@ -43,19 +44,10 @@ pgstat_bktype_io_stats_valid(PgStat_BktypeIO *backend_io,
                {
                        for (int io_op = 0; io_op < IOOP_NUM_TYPES; io_op++)
                        {
-                               /* we do track it */
-                               if (pgstat_tracks_io_op(bktype, io_object, io_context, io_op))
-                               {
-                                       /* ensure that if IO times are non-zero, counts are > 0 */
-                                       if (backend_io->times[io_object][io_context][io_op] != 0 &&
-                                               backend_io->counts[io_object][io_context][io_op] <= 0)
-                                               return false;
-
-                                       continue;
-                               }
-
                                /* we don't track it, and it is not 0 */
-                               if (backend_io->counts[io_object][io_context][io_op] != 0)
+                               if (!pgstat_tracks_io_op(bktype, io_object, io_context, io_op) &&
+                                       (backend_io->counts[io_object][io_context][io_op] != 0 ||
+                                        backend_io->times[io_object][io_context][io_op] != 0))
                                        return false;
                        }
                }