Re: per backend I/O statistics

On Mon, Nov 25, 2024 at 03:47:59PM +0000, Bertrand Drouvot wrote:
> === Remarks
>
> R1: as compared to v5, v6 removes the per-backend I/O stats reset from
> pg_stat_reset_shared(). I think it makes more sense that way, since we are
> adding pg_stat_reset_single_backend_io_counters(). The per-backend I/O stats
> behaves then as the subscription stats as far the reset is concerned.

Makes sense to me to not include that in pg_stat_reset_shared().

> R2: as we can't merge the flush cb anymore, only the patches related to
> the stats_fetch_consistency/'snapshot' are missing in v6 (as compared to v5).
> I propose to re-submit them, re-start the discussion once 0001 goes in.

Yeah, thanks. I should think more about this part, but I'm still kind
of unconvinced. Let's do things step by step. For now, I have looked
at v6.

+ view. The function does not return I/O statistics for the checkpointer,
+ the background writer, the startup process and the autovacuum launcher
+ as they are already visible in the <link linkend="monitoring-pg-stat-io-view"> <structname>pg_stat_io</structname></link>
+ view and there is only one of those.

This last sentence seems unnecessary? The function is named
"backend", and well, all these processes are not backends.

+ /*
+ * Maybe an auxiliary process? That should not be possible, due to
+ * pgstat_tracks_per_backend_bktype() though.
+ */
+ if (proc == NULL)
+ proc = AuxiliaryPidGetProc(backend_pid);
[...]
+ /*
+ * Maybe an auxiliary process? That should not be possible, due to
+ * pgstat_tracks_per_backend_bktype() though.
+ */
+ if (proc == NULL)
+ proc = AuxiliaryPidGetProc(pid);

This does not seem right. Shouldn't we return immediately if
BackendPidGetProc() finds nothing matching with the PID?

+ /* Look for the backend type */
+ for (curr_backend = 1; curr_backend <= num_backends; curr_backend++)
+ {
+ LocalPgBackendStatus *local_beentry;
+ PgBackendStatus *beentry;
+
+ /* Get the next one in the list */
+ local_beentry = pgstat_get_local_beentry_by_index(curr_backend);
+ beentry = &local_beentry->backendStatus;
+
+ /* looking for specific PID, ignore all the others */
+ if (beentry->st_procpid != pid)
+ continue;
+
+ bktype = beentry->st_backendType;
+ break;
+ }

Sounds to me that the backend type is not strictly required in this
function call if pg_stat_activity can tell already that?

+ (void) pgstat_per_backend_flush_cb(entry_ref, nowait);

I'd recommend to not directly call the callback, use a wrapper
function instead if need be.

pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
instr_time start_time, uint32 cnt)
{
+
if (track_io_timing)

Noise diff.

/*
- * Simpler wrapper of pgstat_io_flush_cb()
+ * Simpler wrapper of pgstat_io_flush_cb() and pgstat_per_backend_flush_cb().
*/
void
pgstat_flush_io(bool nowait)

This is also called in the checkpointer and the bgwriter and the
walwriter via pgstat_report_wal(), which is kind of useless. Perhaps
just use a different, separate function instead and use that where it
makes sense (per se also the argument of upthread that backend stats
may not be only IO-related..).

Sounds to me that PgStat_BackendPendingIO should be
PgStat_BackendPendingStats?

+{ oid => '8806', descr => 'statistics: per backend IO statistics',
+ proname => 'pg_stat_get_backend_io', prorows => '5', proretset => 't',

Similarly, s/pg_stat_get_backend_io/pg_stat_get_backend_stats/?

+ descr => 'statistics: reset collected IO statistics for a single backend',
+ proname => 'pg_stat_reset_single_backend_io_counters', provolatile => 'v',

And here, pg_stat_reset_backend_stats?

#define PGSTAT_KIND_SUBSCRIPTION 5 /* per-subscription statistics */
+#define PGSTAT_KIND_PER_BACKEND 6

Missing one comment here.

FWIW, I'm so-so about the addition of pg_my_stat_io, knowing that
pg_stat_get_backend_io(NULL/pg_backend_pid()) does the same job. I
would just add a note in the docs with a query showing how to use it
with pg_stat_activity. An example with LATERAL, doing the same work:
select a.pid, s.* from pg_stat_activity as a,
lateral pg_stat_get_backend_io(a.pid) as s
where pid = pg_backend_pid();
--
Michael