Re: Show WAL write and fsync stats in pg_stat_io

On Fri, Dec 01, 2023 at 12:02:05PM +0300, Nazir Bilal Yavuz wrote:
> Thanks for all the feedback. I am sharing the new version of the patchset.
>
> - I didn't move 'PendingWalStats.wal_sync' out from the
> 'pgstat_count_io_op_n' function because they count the same thing
> (block vs system calls) but I agree that this doesn't look good.

-       if (io_op == IOOP_WRITE || io_op == IOOP_EXTEND)
+       if (io_op == IOOP_EXTEND || io_op == IOOP_WRITE)

Unrelated diff.

+   if (io_object == IOOBJECT_WAL && io_context == IOCONTEXT_NORMAL &&
+       io_op == IOOP_FSYNC)
+       PendingWalStats.wal_sync += cnt;

Nah, I really don't think that adding this dependency within
pg_stat_io is a good idea.

-   PendingWalStats.wal_sync++;
+   pgstat_count_io_op_time(IOOBJECT_WAL, IOCONTEXT_NORMAL, IOOP_FSYNC,
+                           io_start, 1);

This is the only caller where this matters, and the count is always 1.

+    no_wal_normal_read = bktype == B_AUTOVAC_LAUNCHER ||
+        bktype == B_AUTOVAC_WORKER || bktype == B_BACKEND ||
+        bktype == B_BG_WORKER || bktype == B_BG_WRITER ||
+        bktype == B_CHECKPOINTER || bktype == B_WAL_RECEIVER ||
+        bktype == B_WAL_SENDER || bktype == B_WAL_WRITER;
+
+    if (no_wal_normal_read &&
+        (io_object == IOOBJECT_WAL &&
+         io_op == IOOP_READ))
+        return false;

This may be more readable if an enum is applied, without a default
clause so as it would not be forgotten if a new type is added, perhaps
in its own little routine.

-   if (track_io_timing)
+   if (track_io_timing || track_wal_io_timing)
        INSTR_TIME_SET_CURRENT(io_start);
    else

This interface from pgstat_prepare_io_time() is not really good,
because we could finish by setting io_start in the existing code paths
calling this routine even if track_io_timing is false when
track_wal_io_timing is true.  Why not changing this interface a bit
and pass down a GUC (track_io_timing or track_wal_io_timing) as an
argument of the function depending on what we expect to trigger the
timings?

-    /* Convert counters from microsec to millisec for display */
-    values[6] = Float8GetDatum(((double) wal_stats->wal_write_time) / 1000.0);
-    values[7] = Float8GetDatum(((double) wal_stats->wal_sync_time) / 1000.0);
+    /*
+     * There is no need to calculate timings for both pg_stat_wal and
+     * pg_stat_io. So, fetch timings from pg_stat_io to make stats gathering
+     * cheaper. Note that, since timings are fetched from pg_stat_io;
+     * pg_stat_reset_shared('io') will reset pg_stat_wal's timings too.
+     *
+     * Convert counters from microsec to millisec for display
+     */
+    values[6] = Float8GetDatum(pg_stat_get_io_time(IOOBJECT_WAL,
+                                                   IOCONTEXT_NORMAL,
+                                                   IOOP_WRITE));
+    values[7] = Float8GetDatum(pg_stat_get_io_time(IOOBJECT_WAL,
+                                                   IOCONTEXT_NORMAL,
+                                                   IOOP_FSYNC));

Perhaps it is simpler to remove these columns from pg_stat_get_wal()
and plug an SQL upgrade to the view definition of pg_stat_wal?

+int
+pgstat_get_io_op_bytes(IOObject io_object, IOContext io_context)

This interface looks like a good idea even if there is only one
caller.

Finding a good balance between the subroutines, the two GUCs, the
contexts, the I/O operation type and the objects is the tricky part of
this patch.  If the dependency to PendingWalStats is removed and if
the interface of pgstat_prepare_io_time is improved, things are a bit
cleaner, but it feels like we could do more..  Nya.
--
Michael