On Wed, Jan 08, 2025 at 11:11:59AM +0000, Bertrand Drouvot wrote: > Yeah, that's more elegant as it also means that the main callback will not > change > (should we add even more stats in the future). Done that way in v2 attached.
I've put my hands on v2-0002 to begin with something. +/* flag bits for different types of statistics to flush */ +#define PGSTAT_FLUSH_IO (1 << 0) /* Flush I/O statistics */ +#define PGSTAT_FLUSH_ALL (PGSTAT_FLUSH_IO) These are located and used only in pgstat_backend.c. It seems to me that we'd better declare them in pgstat_internal.h and extend the existing pgstat_flush_backend() with an argument so as callers can do what they want. + /* Get our own entry_ref if not provided */ + if (!entry_ref) + entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_BACKEND, InvalidOid, + MyProcNumber, false, NULL); This relates to the previous remark, actually, where I think that it is cleaner to have pgstat_flush_backend() do pgstat_get_entry_ref(), same way as HEAD, and just pass down the flags. pgstat_flush_backend cannot call directly pgstat_backend_flush_cb(), of course, so I've settled down to a new pgstat_flush_backend_entry() that handles the entry locking. This comes at the cost of pgstat_flush_backend_entry() requiring an extra pgstat_tracks_backend_bktype(), which is not a big issue, and the patch gets a bit shorter. -- Michael
From eba357e4e2712cd73fdad585f6d8088de0dbaccc Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Mon, 6 Jan 2025 08:44:29 +0000
Subject: [PATCH v3] PGSTAT_KIND_BACKEND code refactoring
This commit refactors some come related to per backend statistics. It makes
the code more generic or more IO statistics focused as it will be used in a
follow-up commit that will introduce per backend WAL statistics.
---
src/include/pgstat.h | 6 +-
src/include/utils/pgstat_internal.h | 7 +-
src/backend/utils/activity/pgstat.c | 2 +-
src/backend/utils/activity/pgstat_backend.c | 69 ++++++++++++++------
src/backend/utils/activity/pgstat_io.c | 8 +--
src/backend/utils/activity/pgstat_relation.c | 4 +-
src/backend/utils/adt/pgstatfuncs.c | 2 +-
src/tools/pgindent/typedefs.list | 1 +
8 files changed, 68 insertions(+), 31 deletions(-)
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 0d8427f27d1..6631bd2d730 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -381,7 +381,7 @@ typedef PgStat_PendingIO PgStat_BackendPendingIO;
typedef struct PgStat_Backend
{
TimestampTz stat_reset_timestamp;
- PgStat_BktypeIO stats;
+ PgStat_BktypeIO io_stats;
} PgStat_Backend;
typedef struct PgStat_StatDBEntry
@@ -523,6 +523,10 @@ typedef struct PgStat_PendingWalStats
instr_time wal_sync_time;
} PgStat_PendingWalStats;
+typedef struct PgStat_BackendPending
+{
+ PgStat_BackendPendingIO pending_io;
+} PgStat_BackendPending;
/*
* Functions in pgstat.c
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 52eb008710f..4bb8e5c53ab 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -613,9 +613,12 @@ extern void pgstat_archiver_snapshot_cb(void);
* Functions in pgstat_backend.c
*/
-extern void pgstat_flush_backend(bool nowait);
+/* flags for pgstat_flush_backend() */
+#define PGSTAT_BACKEND_FLUSH_IO (1 << 0) /* Flush I/O statistics */
+#define PGSTAT_BACKEND_FLUSH_ALL (PGSTAT_BACKEND_FLUSH_IO)
-extern PgStat_BackendPendingIO *pgstat_prep_backend_pending(ProcNumber procnum);
+extern void pgstat_flush_backend(bool nowait, bits32 flags);
+extern PgStat_BackendPending *pgstat_prep_backend_pending(ProcNumber procnum);
extern bool pgstat_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait);
extern void pgstat_backend_reset_timestamp_cb(PgStatShared_Common *header, TimestampTz ts);
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 16a03b8ce15..34520535d54 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -370,7 +370,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_size = sizeof(PgStatShared_Backend),
.shared_data_off = offsetof(PgStatShared_Backend, stats),
.shared_data_len = sizeof(((PgStatShared_Backend *) 0)->stats),
- .pending_size = sizeof(PgStat_BackendPendingIO),
+ .pending_size = sizeof(PgStat_BackendPending),
.flush_pending_cb = pgstat_backend_flush_cb,
.reset_timestamp_cb = pgstat_backend_reset_timestamp_cb,
diff --git a/src/backend/utils/activity/pgstat_backend.c b/src/backend/utils/activity/pgstat_backend.c
index 1f91bfef0a3..3972bcf9456 100644
--- a/src/backend/utils/activity/pgstat_backend.c
+++ b/src/backend/utils/activity/pgstat_backend.c
@@ -39,23 +39,21 @@ pgstat_fetch_stat_backend(ProcNumber procNumber)
}
/*
- * Flush out locally pending backend statistics
- *
- * If no stats have been recorded, this function returns false.
+ * Flush out locally pending backend IO statistics. Locking is managed
+ * by the caller.
*/
-bool
-pgstat_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
+static void
+pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_ref)
{
- 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;
-
- shbackendioent = (PgStatShared_Backend *) entry_ref->shared_stats;
- bktype_shstats = &shbackendioent->stats.stats;
- pendingent = (PgStat_BackendPendingIO *) entry_ref->pending;
+ shbackendent = (PgStatShared_Backend *) entry_ref->shared_stats;
+ pendingent = (PgStat_BackendPending *) entry_ref->pending;
+ bktype_shstats = &shbackendent->stats.io_stats;
+ pending_io = &pendingent->pending_io;
for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
{
@@ -66,15 +64,33 @@ pgstat_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
instr_time time;
bktype_shstats->counts[io_object][io_context][io_op] +=
- pendingent->counts[io_object][io_context][io_op];
+ pending_io->counts[io_object][io_context][io_op];
- time = pendingent->pending_times[io_object][io_context][io_op];
+ time = pending_io->pending_times[io_object][io_context][io_op];
bktype_shstats->times[io_object][io_context][io_op] +=
INSTR_TIME_GET_MICROSEC(time);
}
}
}
+}
+
+/*
+ * Wrapper routine to flush backend statistics.
+ */
+static bool
+pgstat_flush_backend_entry(PgStat_EntryRef *entry_ref, bool nowait,
+ bits32 flags)
+{
+ if (!pgstat_tracks_backend_bktype(MyBackendType))
+ return false;
+
+ if (!pgstat_lock_entry(entry_ref, nowait))
+ return false;
+
+ /* Flush requested statistics */
+ if (flags & PGSTAT_BACKEND_FLUSH_IO)
+ pgstat_flush_backend_entry_io(entry_ref);
pgstat_unlock_entry(entry_ref);
@@ -82,10 +98,23 @@ pgstat_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
}
/*
- * Simpler wrapper of pgstat_backend_flush_cb()
+ * Callback to flush out locally pending backend statistics.
+ *
+ * If no stats have been recorded, this function returns false.
+ */
+bool
+pgstat_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
+{
+ return pgstat_flush_backend_entry(entry_ref, nowait, PGSTAT_BACKEND_FLUSH_ALL);
+}
+
+/*
+ * Flush out locally pending backend statistics
+ *
+ * "flags" parameter controls which statistics to flush.
*/
void
-pgstat_flush_backend(bool nowait)
+pgstat_flush_backend(bool nowait, bits32 flags)
{
PgStat_EntryRef *entry_ref;
@@ -94,7 +123,7 @@ pgstat_flush_backend(bool nowait)
entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_BACKEND, InvalidOid,
MyProcNumber, false, NULL);
- (void) pgstat_backend_flush_cb(entry_ref, nowait);
+ (void) pgstat_flush_backend_entry(entry_ref, nowait, flags);
}
/*
@@ -119,9 +148,9 @@ pgstat_create_backend(ProcNumber procnum)
}
/*
- * Find or create a local PgStat_BackendPendingIO entry for proc number.
+ * Find or create a local PgStat_BackendPending entry for proc number.
*/
-PgStat_BackendPendingIO *
+PgStat_BackendPending *
pgstat_prep_backend_pending(ProcNumber procnum)
{
PgStat_EntryRef *entry_ref;
diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index f9a1f91dba8..a7445995d32 100644
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -81,10 +81,10 @@ pgstat_count_io_op_n(IOObject io_object, IOContext io_context, IOOp io_op, uint3
if (pgstat_tracks_backend_bktype(MyBackendType))
{
- PgStat_PendingIO *entry_ref;
+ PgStat_BackendPending *entry_ref;
entry_ref = pgstat_prep_backend_pending(MyProcNumber);
- entry_ref->counts[io_object][io_context][io_op] += cnt;
+ entry_ref->pending_io.counts[io_object][io_context][io_op] += cnt;
}
PendingIOStats.counts[io_object][io_context][io_op] += cnt;
@@ -151,10 +151,10 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
if (pgstat_tracks_backend_bktype(MyBackendType))
{
- PgStat_PendingIO *entry_ref;
+ PgStat_BackendPending *entry_ref;
entry_ref = pgstat_prep_backend_pending(MyProcNumber);
- INSTR_TIME_ADD(entry_ref->pending_times[io_object][io_context][io_op],
+ INSTR_TIME_ADD(entry_ref->pending_io.pending_times[io_object][io_context][io_op],
io_time);
}
}
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 2cc304f8812..09247ba0971 100644
--- a/src/backend/utils/activity/pgstat_relation.c
+++ b/src/backend/utils/activity/pgstat_relation.c
@@ -264,7 +264,7 @@ pgstat_report_vacuum(Oid tableoid, bool shared,
* VACUUM command has processed all tables and committed.
*/
pgstat_flush_io(false);
- pgstat_flush_backend(false);
+ pgstat_flush_backend(false, PGSTAT_BACKEND_FLUSH_IO);
}
/*
@@ -351,7 +351,7 @@ pgstat_report_analyze(Relation rel,
/* see pgstat_report_vacuum() */
pgstat_flush_io(false);
- pgstat_flush_backend(false);
+ pgstat_flush_backend(false, PGSTAT_BACKEND_FLUSH_IO);
}
/*
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 3245f3a8d8a..5f8d20a406d 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1544,7 +1544,7 @@ pg_stat_get_backend_io(PG_FUNCTION_ARGS)
if (bktype == B_INVALID)
return (Datum) 0;
- bktype_stats = &backend_stats->stats;
+ bktype_stats = &backend_stats->io_stats;
/*
* In Assert builds, we can afford an extra loop through all of the
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9f83ecf181f..f15526236a3 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2140,6 +2140,7 @@ PgStatShared_Subscription
PgStatShared_Wal
PgStat_ArchiverStats
PgStat_Backend
+PgStat_BackendPending
PgStat_BackendPendingIO
PgStat_BackendSubEntry
PgStat_BgWriterStats
--
2.47.1
signature.asc
Description: PGP signature
