Re: per backend I/O statistics

Hi,

On Thu, Dec 12, 2024 at 01:52:03PM +0900, Michael Paquier wrote:
> On Mon, Nov 25, 2024 at 03:47:59PM +0000, Bertrand Drouvot wrote:

> +        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.

Yeah, but you can find their pid through pg_stat_activity for which the pid field
description is "Process ID of this backend". Also we can find "backend_type" in
both pg_stat_activity and pg_stat_io, so I think this last sentence could help 
to avoid any confusion though.

> +    /*
> +     * 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?

Yeah, that would work. I was keeping the AuxiliaryPidGetProc() calls just in case
we want to add the Aux processes back in the future. Replaced with a comment
in v7 attached instead.

> 
> +	/* 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?

Yeah, but it's needed in pg_stat_get_backend_io() for the stats filtering (to
display only those linked to this backend type), later in the function here:

+    /*
+     * Some combinations of BackendType, IOObject, and IOContext are
+     * not valid for any type of IOOp. In such cases, omit the entire
+     * row from the view.
+     */
+    if (!pgstat_tracks_io_object(bktype, io_obj, io_context))
+          continue;

OTOH, now that we get rid of the AuxiliaryPidGetProc() call in
pg_stat_reset_single_backend_io_counters() we can also remove the need to
look for the backend type in this function.

> 
> +       (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.

Makes sense, created its own callback in pgstat_backend.c. Bonus point, it avoids
unnecessary pgstat_tracks_per_backend_bktype() checks in :

pgstat_report_bgwriter()
pgstat_report_checkpointer()
pgstat_report_wal()

as there is no attempt to call the callback anymore in those places.

>  /*
> - * 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..).

Yeah, done that way. 

BTW, not related to this particular patch but I realized that pgstat_flush_io()
is called for the walwriter. Indeed, it's coming from the pgstat_report_wal()
call in WalWriterMain(). That can not report any I/O stats activity (as the
walwriter is not part of the I/O stats tracking, see pgstat_tracks_io_bktype()).

So it looks like, we could move pgstat_flush_io() outside of pgstat_report_wal()
and add the pgstat_flush_io() calls only where they need to be made (and so, not
in WalWriterMain()).

Maybe a dedicated thread is worth it for that, thoughts?

> Sounds to me that PgStat_BackendPendingIO should be
> PgStat_BackendPendingStats?

Yeah, can do that as that's what is being used for the pending_size for
example.

OTOH, it's clear that this one in pgstat_io.c has to be "linked" to an "IO" 
related one:

"
typedef PgStat_BackendPendingStats PgStat_PendingIO;
"

The right way would probably be to do something like:

"
typedef struct PgStat_BackendPendingStats {
    PgStat_BackendPendingIO pendingio;  
} PgStat_BackendPendingStats;
"

But I'm not sure that's worth it until we don't have a need to add more
pending stats per-backend, thoughts?

> +{ 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/?

I think that's fine to keep pg_stat_get_backend_io(). If we add more per-backend
stats in the future then we could add "dedicated" get functions too and a generic
one retrieving all of them.

> +  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?

Same as above, we could imagine that in the future the backend would get mutiple
stats and that one would want to reset only the I/O ones for example.

>  #define PGSTAT_KIND_SUBSCRIPTION   5   /* per-subscription statistics */
> +#define PGSTAT_KIND_PER_BACKEND    6 
> 
> Missing one comment here.

Yeap.

> 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.

Okay, I don't have a strong opinion about that. Removed in v7.

> 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();

I'm not sure it's worth it. I think that's clear that to get our own stats
then we need to provide our own backend pid. For example pg_stat_get_activity()
does not provide such an example using pg_stat_activity or using something like 
pg_stat_get_activity(pg_backend_pid()).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com