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
signature.asc
Description: PGP signature
