On Wed, Jul 23, 2025 at 12:00:55PM -0400, Andres Freund wrote: > On 2025-07-23 09:54:12 +0900, Michael Paquier wrote: >> Noted. I was wondering originally if the threshold for the builtin >> and custom kinds should be lowered, as well. Perhaps that's just too >> optimistic, so we can first lower things. Say PGSTAT_KIND_CUSTOM_MIN >> to 32 and PGSTAT_KIND_MAX to 48 or so to begin with? I don't see a >> strict policy here, but these would make the whole cheaper to begin >> with, with enough margin for any new stats kinds. > > Yes, 256 seems pointlessly large.
I still cannot put my finger on what a good number should be, but I've settled to something that I think should be good enough for an initial release: PGSTAT_KIND_CUSTOM_MIN 128 -> 24 PGSTAT_KIND_MAX 256 -> 32 These numbers mean that we have enough room for 7 more builtins kinds, same for custom kinds. Even if this is an issue, this could always be moved up at some point even across major releases, even if that would mean for extension developers to switch to a different ID. >> Then, about the loop used here, I'd be OK to keep the past shortcuts >> for the builtin fixed-sized stats kinds with the requirement that new >> builtin stats kinds need to hack into pgstat_report_stat() themselves >> on efficiency grounds, combined with a second loop for custom kinds, >> taken only if pgstat_register_kind() has been used by tracking if >> that's the case in a flag. Do you have a specific preference here? > > I think the downstream approach of having a flag that says if there are *any* > pending stats is better. Works for me. I am finishing with the attached, meaning that workloads that trigger no stats can take the fast-exit path with a single boolean check. What do you think? -- Michael
From 05ded16fe6ffd87835ff29b0136e46bb76a81687 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Thu, 24 Jul 2025 08:18:06 +0900 Subject: [PATCH 1/2] Lower bounds of pgstats kinds This changes stats kinds to have the following bounds, making their handling in core cheaper by default: - PGSTAT_KIND_CUSTOM_MIN 128 -> 24 - PGSTAT_KIND_MAX 256 -> 32 This should be enough to leave some room for the following years for built-in and custom stats kinds. At least that should be enough to start with this facility for extension developers. --- src/include/utils/pgstat_kind.h | 6 +++--- src/test/modules/injection_points/injection_stats.c | 2 +- src/test/modules/injection_points/injection_stats_fixed.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/include/utils/pgstat_kind.h b/src/include/utils/pgstat_kind.h index f44169fd5a3c..eb5f0b3ae6db 100644 --- a/src/include/utils/pgstat_kind.h +++ b/src/include/utils/pgstat_kind.h @@ -18,7 +18,7 @@ /* Range of IDs allowed, for built-in and custom kinds */ #define PGSTAT_KIND_MIN 1 /* Minimum ID allowed */ -#define PGSTAT_KIND_MAX 256 /* Maximum ID allowed */ +#define PGSTAT_KIND_MAX 32 /* Maximum ID allowed */ /* use 0 for INVALID, to catch zero-initialized data */ #define PGSTAT_KIND_INVALID 0 @@ -46,7 +46,7 @@ /* Custom stats kinds */ /* Range of IDs allowed for custom stats kinds */ -#define PGSTAT_KIND_CUSTOM_MIN 128 +#define PGSTAT_KIND_CUSTOM_MIN 24 #define PGSTAT_KIND_CUSTOM_MAX PGSTAT_KIND_MAX #define PGSTAT_KIND_CUSTOM_SIZE (PGSTAT_KIND_CUSTOM_MAX - PGSTAT_KIND_CUSTOM_MIN + 1) @@ -55,7 +55,7 @@ * development and have not reserved their own unique kind ID yet. See: * https://wiki.postgresql.org/wiki/CustomCumulativeStats */ -#define PGSTAT_KIND_EXPERIMENTAL 128 +#define PGSTAT_KIND_EXPERIMENTAL 24 static inline bool pgstat_is_kind_builtin(PgStat_Kind kind) diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c index 14903c629e0d..e3947b23ba57 100644 --- a/src/test/modules/injection_points/injection_stats.c +++ b/src/test/modules/injection_points/injection_stats.c @@ -59,7 +59,7 @@ static const PgStat_KindInfo injection_stats = { /* * Kind ID reserved for statistics of injection points. */ -#define PGSTAT_KIND_INJECTION 129 +#define PGSTAT_KIND_INJECTION 25 /* Track if stats are loaded */ static bool inj_stats_loaded = false; diff --git a/src/test/modules/injection_points/injection_stats_fixed.c b/src/test/modules/injection_points/injection_stats_fixed.c index 3d0c01bdd05a..bc54c79d190b 100644 --- a/src/test/modules/injection_points/injection_stats_fixed.c +++ b/src/test/modules/injection_points/injection_stats_fixed.c @@ -64,7 +64,7 @@ static const PgStat_KindInfo injection_stats_fixed = { /* * Kind ID reserved for statistics of injection points. */ -#define PGSTAT_KIND_INJECTION_FIXED 130 +#define PGSTAT_KIND_INJECTION_FIXED 26 /* Track if fixed-numbered stats are loaded */ static bool inj_fixed_loaded = false; -- 2.50.0
From 6814ddec82e40a14d61ba0a5c93974c76640ab52 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 24 Jul 2025 09:03:53 +0900
Subject: [PATCH 2/2] Fix performance regression with pgstats reports
The introduction of the new callback to control the flush of pending
entries is proving to show up high in workloads that do not report any
stats (read only, no function calls, etc).
This commit switches the code to use a cheaper approach, with a single
boolean flag that can be switched to "true" by any fixed-numbered stats
kinds to force pgstat_report_stat() to go through one round of reports.
The flag can only be reset by pgstat_report_stat() once a full round of
reports is done.
The callback have_static_pending_cb can be removed as an effect of that.
---
src/include/utils/pgstat_internal.h | 30 +++++++++---------
src/backend/access/transam/xlog.c | 10 ++++++
src/backend/utils/activity/pgstat.c | 35 ++++++---------------
src/backend/utils/activity/pgstat_backend.c | 14 ++-------
src/backend/utils/activity/pgstat_io.c | 10 +-----
src/backend/utils/activity/pgstat_slru.c | 10 +-----
src/backend/utils/activity/pgstat_wal.c | 20 ++++++------
7 files changed, 49 insertions(+), 80 deletions(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index d5557e6e998c..c4c82c1235a6 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -295,19 +295,12 @@ typedef struct PgStat_KindInfo
*
* Returns true if some of the stats could not be flushed, due to lock
* contention for example. Optional.
+ *
+ * "pgstat_report_fixed" needs to be set to trigger the flush of pending
+ * stats.
*/
bool (*flush_static_cb) (bool nowait);
- /*
- * For fixed-numbered or variable-numbered statistics: Check for pending
- * stats in need of flush with flush_static_cb, when these do not use
- * PgStat_EntryRef->pending.
- *
- * Returns true if there are any stats pending for flush, triggering
- * flush_static_cb. Optional.
- */
- bool (*have_static_pending_cb) (void);
-
/*
* For fixed-numbered statistics: Reset All.
*/
@@ -627,7 +620,6 @@ extern void pgstat_archiver_snapshot_cb(void);
extern bool pgstat_flush_backend(bool nowait, bits32 flags);
extern bool pgstat_backend_flush_cb(bool nowait);
-extern bool pgstat_backend_have_pending_cb(void);
extern void pgstat_backend_reset_timestamp_cb(PgStatShared_Common *header,
TimestampTz ts);
@@ -676,7 +668,6 @@ extern bool pgstat_function_flush_cb(PgStat_EntryRef *entry_ref, 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);
@@ -738,7 +729,6 @@ extern PgStatShared_Common *pgstat_init_entry(PgStat_Kind kind,
* Functions in pgstat_slru.c
*/
-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);
@@ -750,7 +740,6 @@ extern void pgstat_slru_snapshot_cb(void);
*/
extern void pgstat_wal_init_backend_cb(void);
-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);
@@ -780,6 +769,19 @@ extern void pgstat_create_transactional(PgStat_Kind kind, Oid dboid, uint64 obji
extern PGDLLIMPORT PgStat_LocalState pgStatLocal;
+/*
+ * Track if *any* pending fixed-numbered statistics should be flushed to
+ * shared memory.
+ *
+ * This flag can be switched to true by fixed-numbered statistics to let
+ * pgstat_report_stat() know if it needs to go through one round of
+ * reports, giving the possibility of a fast-path exit if there are no
+ * pending fixed-numbered statistics.
+ *
+ * Statistics callbacks should never reset this flag; pgstat_report_stat()
+ * is in charge of doing that.
+ */
+extern PGDLLIMPORT bool pgstat_report_fixed;
/*
* Implementation of inline functions declared above.
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index eefffc4277a1..b0891998b243 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -96,6 +96,7 @@
#include "utils/guc_hooks.h"
#include "utils/guc_tables.h"
#include "utils/injection_point.h"
+#include "utils/pgstat_internal.h"
#include "utils/ps_status.h"
#include "utils/relmapper.h"
#include "utils/snapmgr.h"
@@ -1091,6 +1092,9 @@ XLogInsertRecord(XLogRecData *rdata,
pgWalUsage.wal_bytes += rechdr->xl_tot_len;
pgWalUsage.wal_records++;
pgWalUsage.wal_fpi += num_fpi;
+
+ /* Required for the flush of pending stats WAL data */
+ pgstat_report_fixed = true;
}
return EndPos;
@@ -2108,6 +2112,12 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
LWLockRelease(WALWriteLock);
pgWalUsage.wal_buffers_full++;
TRACE_POSTGRESQL_WAL_BUFFER_WRITE_DIRTY_DONE();
+
+ /*
+ * Required for the flush of pending stats WAL data, per
+ * update of pgWalUsage.
+ */
+ pgstat_report_fixed = true;
}
}
}
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 8b57845e8709..0174df929481 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -212,6 +212,11 @@ int pgstat_fetch_consistency = PGSTAT_FETCH_CONSISTENCY_CACHE;
PgStat_LocalState pgStatLocal;
+/*
+ * Track pending reports for fixed-numbered stats, used by
+ * pgstat_report_stat().
+ */
+bool pgstat_report_fixed = false;
/* ----------
* Local data
@@ -370,7 +375,6 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_off = offsetof(PgStatShared_Backend, stats),
.shared_data_len = sizeof(((PgStatShared_Backend *) 0)->stats),
- .have_static_pending_cb = pgstat_backend_have_pending_cb,
.flush_static_cb = pgstat_backend_flush_cb,
.reset_timestamp_cb = pgstat_backend_reset_timestamp_cb,
},
@@ -437,7 +441,6 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_IO *) 0)->stats),
.flush_static_cb = pgstat_io_flush_cb,
- .have_static_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,
@@ -455,7 +458,6 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_SLRU *) 0)->stats),
.flush_static_cb = pgstat_slru_flush_cb,
- .have_static_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,
@@ -474,7 +476,6 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.init_backend_cb = pgstat_wal_init_backend_cb,
.flush_static_cb = pgstat_wal_flush_cb,
- .have_static_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,
@@ -708,29 +709,10 @@ pgstat_report_stat(bool force)
}
/* Don't expend a clock check if nothing to do */
- if (dlist_is_empty(&pgStatPending))
+ if (dlist_is_empty(&pgStatPending) &&
+ !pgstat_report_fixed)
{
- bool do_flush = false;
-
- /* Check for pending 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->have_static_pending_cb)
- continue;
-
- if (kind_info->have_static_pending_cb())
- {
- do_flush = true;
- break;
- }
- }
-
- if (!do_flush)
- return 0;
+ return 0;
}
/*
@@ -815,6 +797,7 @@ pgstat_report_stat(bool force)
}
pending_since = 0;
+ pgstat_report_fixed = false;
return 0;
}
diff --git a/src/backend/utils/activity/pgstat_backend.c b/src/backend/utils/activity/pgstat_backend.c
index 51256277e8d3..8714a85e2d93 100644
--- a/src/backend/utils/activity/pgstat_backend.c
+++ b/src/backend/utils/activity/pgstat_backend.c
@@ -66,6 +66,7 @@ pgstat_count_backend_io_op_time(IOObject io_object, IOContext io_context,
io_time);
backend_has_iostats = true;
+ pgstat_report_fixed = true;
}
void
@@ -81,6 +82,7 @@ pgstat_count_backend_io_op(IOObject io_object, IOContext io_context,
PendingBackendStats.pending_io.bytes[io_object][io_context][io_op] += bytes;
backend_has_iostats = true;
+ pgstat_report_fixed = true;
}
/*
@@ -301,18 +303,6 @@ pgstat_flush_backend(bool nowait, bits32 flags)
return false;
}
-/*
- * Check if there are any backend stats waiting for flush.
- */
-bool
-pgstat_backend_have_pending_cb(void)
-{
- if (!pgstat_tracks_backend_bktype(MyBackendType))
- return false;
-
- return (backend_has_iostats || pgstat_backend_wal_have_pending());
-}
-
/*
* Callback to flush out locally pending backend statistics.
*
diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index d8d26379a571..13ae57ed6498 100644
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -80,6 +80,7 @@ pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op,
pgstat_count_backend_io_op(io_object, io_context, io_op, cnt, bytes);
have_iostats = true;
+ pgstat_report_fixed = true;
}
/*
@@ -167,15 +168,6 @@ 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()
*/
diff --git a/src/backend/utils/activity/pgstat_slru.c b/src/backend/utils/activity/pgstat_slru.c
index b9e940dde45b..7bd8744accb0 100644
--- a/src/backend/utils/activity/pgstat_slru.c
+++ b/src/backend/utils/activity/pgstat_slru.c
@@ -143,15 +143,6 @@ 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
*
@@ -247,6 +238,7 @@ get_slru_entry(int slru_idx)
Assert((slru_idx >= 0) && (slru_idx < SLRU_NUM_ELEMENTS));
have_slrustats = true;
+ pgstat_report_fixed = true;
return &pending_SLRUStats[slru_idx];
}
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index 16a1ecb4d90d..0d04480d2f6d 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;
}
+/*
+ * To determine whether WAL usage happened.
+ */
+static inline bool
+pgstat_wal_have_pending(void)
+{
+ return pgWalUsage.wal_records != prevWalUsage.wal_records;
+}
+
/*
* Calculate how much WAL usage counters have increased by subtracting the
* previous counters from the current ones.
@@ -92,7 +101,7 @@ pgstat_wal_flush_cb(bool nowait)
* This function can be called even if nothing at all has happened. Avoid
* taking lock for nothing in that case.
*/
- if (!pgstat_wal_have_pending_cb())
+ if (!pgstat_wal_have_pending())
return false;
/*
@@ -136,15 +145,6 @@ pgstat_wal_init_backend_cb(void)
prevWalUsage = pgWalUsage;
}
-/*
- * To determine whether WAL usage happened.
- */
-bool
-pgstat_wal_have_pending_cb(void)
-{
- return pgWalUsage.wal_records != prevWalUsage.wal_records;
-}
-
void
pgstat_wal_init_shmem_cb(void *stats)
{
--
2.50.0
signature.asc
Description: PGP signature
