Hi all, The last TODO item I had in my bucket about the generalization of pgstats is the option to a better control on the flush of the stats depending on the kind for fixed-numbered stats. Currently, this is controlled by pgstat_report_stat(), that includes special handling for WAL, IO and SLRU stats, with two generic concepts: - Check if there are pending entries, allowing a fast-path exit. - Do the actual flush, with a recheck on pending entries.
SLRU and IO control that with one variable each, and WAL uses a routine for the same called pgstat_have_pending_wal(). Please find attached a patch to generalize the concept, with two new callbacks that can be used for fixed-numbered stats. SLRU, IO and WAL are switched to use these (the two pgstat_flush_* routines have been kept on purpose). This brings some clarity in the code, by making have_iostats and have_slrustats static in their respective files. The two pgstat_flush_* wrappers do not need a boolean as return result. Running Postgres on scissors with a read-only workload that does not trigger stats, I was not able to see a difference in runtime, but that was on my own laptop, and I am planning to do more measurements on a bigger machine. This is in the same line of thoughts as the recent thread about the backend init callback, generalizing more the whole facility: https://www.postgresql.org/message-id/ztzr1k4pldewc...@paquier.xyz Like the other one, I wanted to send that a few days ago, but well, life likes going its own ways sometimes. Thanks, -- Michael
From e98f8d50c17cd8fc521bffb4bdc73ef58b7fa430 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 3 Sep 2024 13:36:10 +0900
Subject: [PATCH] Add callbacks to control flush of fixed-numbered stats
This commit adds two callbacks in pgstats:
- have_fixed_pending_cb, to check if a stats kind has any pending stuff
waiting for a flush.
- flush_fixed_cb, to do the flush.
This is used by the SLRU, WAL and IO statistics, generalizing the
concept for all stats kinds (builtin and custom).
---
src/include/utils/pgstat_internal.h | 42 +++++++++-------
src/backend/utils/activity/pgstat.c | 63 +++++++++++++++++++-----
src/backend/utils/activity/pgstat_io.c | 22 ++++++++-
src/backend/utils/activity/pgstat_slru.c | 13 ++++-
src/backend/utils/activity/pgstat_wal.c | 19 +++++--
5 files changed, 119 insertions(+), 40 deletions(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index fb132e439d..d69aa05b1c 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -231,7 +231,7 @@ typedef struct PgStat_KindInfo
/*
* For variable-numbered stats: flush pending stats. Required if pending
- * data is used.
+ * data is used. See flush_fixed_cb for fixed-numbered stats.
*/
bool (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait);
@@ -259,6 +259,19 @@ typedef struct PgStat_KindInfo
*/
void (*init_shmem_cb) (void *stats);
+ /*
+ * For fixed-numbered statistics: Flush pending stats. Returns true if
+ * some of the stats could not be flushed, due to lock contention for
+ * example. Optional.
+ */
+ bool (*flush_fixed_cb) (bool nowait);
+
+ /*
+ * For fixed-numbered statistics: Check for pending stats in need of
+ * flush. Returns true if there are any stats pending for flush. Optional.
+ */
+ bool (*have_fixed_pending_cb) (void);
+
/*
* For fixed-numbered statistics: Reset All.
*/
@@ -603,7 +616,10 @@ extern bool pgstat_function_flush_cb(PgStat_EntryRef *entry_ref, bool nowait);
* Functions in pgstat_io.c
*/
-extern bool pgstat_flush_io(bool nowait);
+extern void pgstat_flush_io(bool nowait);
+
+extern bool pgstat_io_have_pending_cb(void);
+extern bool pgstat_io_flush_cb(bool nowait);
extern void pgstat_io_init_shmem_cb(void *stats);
extern void pgstat_io_reset_all_cb(TimestampTz ts);
extern void pgstat_io_snapshot_cb(void);
@@ -662,7 +678,8 @@ extern PgStatShared_Common *pgstat_init_entry(PgStat_Kind kind,
* Functions in pgstat_slru.c
*/
-extern bool pgstat_slru_flush(bool nowait);
+extern bool pgstat_slru_have_pending_cb(void);
+extern bool pgstat_slru_flush_cb(bool nowait);
extern void pgstat_slru_init_shmem_cb(void *stats);
extern void pgstat_slru_reset_all_cb(TimestampTz ts);
extern void pgstat_slru_snapshot_cb(void);
@@ -672,10 +689,11 @@ extern void pgstat_slru_snapshot_cb(void);
* Functions in pgstat_wal.c
*/
-extern bool pgstat_flush_wal(bool nowait);
extern void pgstat_init_wal(void);
-extern bool pgstat_have_pending_wal(void);
+extern void pgstat_flush_wal(bool nowait);
+extern bool pgstat_wal_have_pending_cb(void);
+extern bool pgstat_wal_flush_cb(bool nowait);
extern void pgstat_wal_init_shmem_cb(void *stats);
extern void pgstat_wal_reset_all_cb(TimestampTz ts);
extern void pgstat_wal_snapshot_cb(void);
@@ -705,20 +723,6 @@ extern void pgstat_create_transactional(PgStat_Kind kind, Oid dboid, Oid objoid)
extern PGDLLIMPORT PgStat_LocalState pgStatLocal;
-/*
- * Variables in pgstat_io.c
- */
-
-extern PGDLLIMPORT bool have_iostats;
-
-
-/*
- * Variables in pgstat_slru.c
- */
-
-extern PGDLLIMPORT bool have_slrustats;
-
-
/*
* Implementation of inline functions declared above.
*/
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index b2ca3f39b7..8cbf706d5b 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -411,6 +411,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_off = offsetof(PgStatShared_IO, stats),
.shared_data_len = sizeof(((PgStatShared_IO *) 0)->stats),
+ .flush_fixed_cb = pgstat_io_flush_cb,
+ .have_fixed_pending_cb = pgstat_io_have_pending_cb,
.init_shmem_cb = pgstat_io_init_shmem_cb,
.reset_all_cb = pgstat_io_reset_all_cb,
.snapshot_cb = pgstat_io_snapshot_cb,
@@ -426,6 +428,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_off = offsetof(PgStatShared_SLRU, stats),
.shared_data_len = sizeof(((PgStatShared_SLRU *) 0)->stats),
+ .flush_fixed_cb = pgstat_slru_flush_cb,
+ .have_fixed_pending_cb = pgstat_slru_have_pending_cb,
.init_shmem_cb = pgstat_slru_init_shmem_cb,
.reset_all_cb = pgstat_slru_reset_all_cb,
.snapshot_cb = pgstat_slru_snapshot_cb,
@@ -441,6 +445,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_off = offsetof(PgStatShared_Wal, stats),
.shared_data_len = sizeof(((PgStatShared_Wal *) 0)->stats),
+ .flush_fixed_cb = pgstat_wal_flush_cb,
+ .have_fixed_pending_cb = pgstat_wal_have_pending_cb,
.init_shmem_cb = pgstat_wal_init_shmem_cb,
.reset_all_cb = pgstat_wal_reset_all_cb,
.snapshot_cb = pgstat_wal_snapshot_cb,
@@ -661,13 +667,37 @@ pgstat_report_stat(bool force)
}
/* Don't expend a clock check if nothing to do */
- if (dlist_is_empty(&pgStatPending) &&
- !have_iostats &&
- !have_slrustats &&
- !pgstat_have_pending_wal())
+ if (dlist_is_empty(&pgStatPending))
{
- Assert(pending_since == 0);
- return 0;
+ bool do_flush = false;
+
+ /* Check for pending fixed-numbered stats */
+ for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
+ {
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+ if (!kind_info)
+ continue;
+ if (!kind_info->fixed_amount)
+ {
+ Assert(kind_info->have_fixed_pending_cb == NULL);
+ continue;
+ }
+ if (!kind_info->have_fixed_pending_cb)
+ continue;
+
+ if (kind_info->have_fixed_pending_cb())
+ {
+ do_flush = true;
+ break;
+ }
+ }
+
+ if (!do_flush)
+ {
+ Assert(pending_since == 0);
+ return 0;
+ }
}
/*
@@ -720,14 +750,23 @@ pgstat_report_stat(bool force)
/* flush database / relation / function / ... stats */
partial_flush |= pgstat_flush_pending_entries(nowait);
- /* flush IO stats */
- partial_flush |= pgstat_flush_io(nowait);
+ /* flush of fixed-numbered stats */
+ for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
+ {
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
- /* flush wal stats */
- partial_flush |= pgstat_flush_wal(nowait);
+ if (!kind_info)
+ continue;
+ if (!kind_info->fixed_amount)
+ {
+ Assert(kind_info->flush_fixed_cb == NULL);
+ continue;
+ }
+ if (!kind_info->flush_fixed_cb)
+ continue;
- /* flush SLRU stats */
- partial_flush |= pgstat_slru_flush(nowait);
+ partial_flush |= kind_info->flush_fixed_cb(nowait);
+ }
last_flush = now;
diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index 8af55989ee..cc2ffc78aa 100644
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -29,7 +29,7 @@ typedef struct PgStat_PendingIO
static PgStat_PendingIO PendingIOStats;
-bool have_iostats = false;
+static bool have_iostats = false;
/*
@@ -161,6 +161,24 @@ pgstat_fetch_stat_io(void)
return &pgStatLocal.snapshot.io;
}
+/*
+ * Check if there any IO stats waiting for flush.
+ */
+bool
+pgstat_io_have_pending_cb(void)
+{
+ return have_iostats;
+}
+
+/*
+ * Simpler wrapper of pgstat_io_flush_cb()
+ */
+void
+pgstat_flush_io(bool nowait)
+{
+ (void) pgstat_io_flush_cb(nowait);
+}
+
/*
* Flush out locally pending IO statistics
*
@@ -170,7 +188,7 @@ pgstat_fetch_stat_io(void)
* acquired. Otherwise, return false.
*/
bool
-pgstat_flush_io(bool nowait)
+pgstat_io_flush_cb(bool nowait)
{
LWLock *bktype_lock;
PgStat_BktypeIO *bktype_shstats;
diff --git a/src/backend/utils/activity/pgstat_slru.c b/src/backend/utils/activity/pgstat_slru.c
index 6f922a85bf..dd6f9f840e 100644
--- a/src/backend/utils/activity/pgstat_slru.c
+++ b/src/backend/utils/activity/pgstat_slru.c
@@ -32,7 +32,7 @@ static void pgstat_reset_slru_counter_internal(int index, TimestampTz ts);
* in order to avoid memory allocation.
*/
static PgStat_SLRUStats pending_SLRUStats[SLRU_NUM_ELEMENTS];
-bool have_slrustats = false;
+static bool have_slrustats = false;
/*
@@ -143,6 +143,15 @@ pgstat_get_slru_index(const char *name)
return (SLRU_NUM_ELEMENTS - 1);
}
+/*
+ * Check if there are any SLRU stats entries waiting for flush.
+ */
+bool
+pgstat_slru_have_pending_cb(void)
+{
+ return have_slrustats;
+}
+
/*
* Flush out locally pending SLRU stats entries
*
@@ -153,7 +162,7 @@ pgstat_get_slru_index(const char *name)
* acquired. Otherwise return false.
*/
bool
-pgstat_slru_flush(bool nowait)
+pgstat_slru_flush_cb(bool nowait)
{
PgStatShared_SLRU *stats_shmem = &pgStatLocal.shmem->slru;
int i;
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index e2a3f6b865..3d76091f71 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -71,6 +71,15 @@ pgstat_fetch_stat_wal(void)
return &pgStatLocal.snapshot.wal;
}
+/*
+ * Simple wrapper of pgstat_wal_flush_cb()
+ */
+void
+pgstat_flush_wal(bool nowait)
+{
+ (void) pgstat_wal_flush_cb(nowait);
+}
+
/*
* Calculate how much WAL usage counters have increased by subtracting the
* previous counters from the current ones.
@@ -79,7 +88,7 @@ pgstat_fetch_stat_wal(void)
* acquired. Otherwise return false.
*/
bool
-pgstat_flush_wal(bool nowait)
+pgstat_wal_flush_cb(bool nowait)
{
PgStatShared_Wal *stats_shmem = &pgStatLocal.shmem->wal;
WalUsage wal_usage_diff = {0};
@@ -92,7 +101,7 @@ pgstat_flush_wal(bool nowait)
* This function can be called even if nothing at all has happened. Avoid
* taking lock for nothing in that case.
*/
- if (!pgstat_have_pending_wal())
+ if (!pgstat_wal_have_pending_cb())
return false;
/*
@@ -141,8 +150,8 @@ void
pgstat_init_wal(void)
{
/*
- * Initialize prevWalUsage with pgWalUsage so that pgstat_flush_wal() can
- * calculate how much pgWalUsage counters are increased by subtracting
+ * Initialize prevWalUsage with pgWalUsage so that pgstat_wal_flush_cb()
+ * can calculate how much pgWalUsage counters are increased by subtracting
* prevWalUsage from pgWalUsage.
*/
prevWalUsage = pgWalUsage;
@@ -156,7 +165,7 @@ pgstat_init_wal(void)
* data pages.
*/
bool
-pgstat_have_pending_wal(void)
+pgstat_wal_have_pending_cb(void)
{
return pgWalUsage.wal_records != prevWalUsage.wal_records ||
PendingWalStats.wal_write != 0 ||
--
2.45.2
signature.asc
Description: PGP signature
