Re: per backend WAL statistics

On Tue, Jan 07, 2025 at 08:48:51AM +0000, Bertrand Drouvot wrote:
> Now that commit 9aea73fc61 added backend-level statistics to pgstats (and
> per backend IO statistics), we can more easily add per backend statistics.
>
> Please find attached a patch to implement $SUBJECT.

I've looked at v1-0002 and v1-0001.

+static void
+pgstat_flush_io_entry(PgStat_EntryRef *entry_ref, bool nowait, bool need_lock)
{
- PgStatShared_Backend *shbackendioent;
- PgStat_BackendPendingIO *pendingent;
+ PgStatShared_Backend *shbackendent;
+ PgStat_BackendPending *pendingent;
PgStat_BktypeIO *bktype_shstats;
+ PgStat_BackendPendingIO *pending_io;

- if (!pgstat_lock_entry(entry_ref, nowait))
- return false;
+ if (need_lock && !pgstat_lock_entry(entry_ref, nowait))
+ return;

The addition of need_lock at this level leads to a result that seems a
bit confusing, where pgstat_backend_flush_cb() passes "false" because
it locks the entry by itself as an effect of v1-0003 with the new area
for WAL. Wouldn't it be cleaner to do an extra pgstat_[un]lock_entry
dance in pgstat_backend_flush_io() instead? Another approach I can
think of that would be slightly cleaner to me is to pass a bits32 to a
single routine that would control if WAL stats, I/O stats or both
should be flushed, keeping pgstat_flush_backend() as name with an
extra argument to decide which parts of the stats should be flushed.

-PgStat_BackendPendingIO *
+PgStat_BackendPending *

This rename makes sense.

#define PG_STAT_GET_WAL_COLS 9
TupleDesc tupdesc;
Datum values[PG_STAT_GET_WAL_COLS] = {0};
bool nulls[PG_STAT_GET_WAL_COLS] = {0};

It feels unnatural to have a PG_STAT_GET_WAL_COLS while it would not
only relate to this function anymore.
--
Michael