Hi,
On Mon, Jan 20, 2025 at 03:34:41PM +0900, Michael Paquier wrote:
> On Sat, Jan 18, 2025 at 05:53:31PM +0900, Michael Paquier wrote:
> > Hmm. Such special complexities in pgstat.c are annoying. There is
> > a stupid thing I am wondering here. For the WAL stats, why couldn't
> > we place some calls of pgstat_prep_backend_pending() in strategic
> > places like XLogBeginInsert() to force all the allocation steps of the
> > pending entry to happen before we would enter the critical sections
> > when doing a WAL insertion? As far as I can see, there is a special
> > case with 2PC where XLogBeginInsert() could be called in a critical
> > section, but that seems to be the only one at quick glance.
>
> I've looked first at this idea over the week-end with a quick hack,
> and it did not finish well.
Yeah, that would probably also mean moving the current WAL pending increments to
the same "new" place (to be consistent) which does not seem great.
> And then it struck me that with a bit of redesign of the callbacks, so
Thanks!
> as the existing flush_fixed_cb and have_fixed_pending_cb are usable
> with variable-numbered stats, we should be able to avoid the exception
> your first version of the patch has introduced. Attached is a patch
> to achieve what I have in mind, which is more generic than your
> previous patch. I've built my stuff as 0002 on top of your 0001.
>
Indeed. Another idea that I had would have been to create another callback,
but I prefer your idea.
=== 1
I think that flush_pending_cb, flush_cb and have_pending_cb are now somehow
confusing (one could think that have_pending_cb is only linked to
flush_pending_cb).
I think that it would be better to make the distinction based on "local/static"
vs "dynamic memory" pending stats instead: I did so in v3 attached, using:
.flush_dynamic_cb(): flushes pending entries tracked in dynamic memory
.flush_static_cb(): flushes pending stats from static/global variable
Also I reworded the comments a bit.
=== 2
- /*
- * Clear out the statistics buffer, so it can be re-used.
- */
- MemSet(&PendingBackendStats.pending_io, 0, sizeof(PgStat_PendingIO));
The inital intent was to clear *only" the pending IO stats, so that each
flush (IO, WAL once implemented,...) could clear the pending stats it is
responsible for.
=== 3
+ /*
+ * Clear out the statistics buffer, so it can be re-used.
+ */
+ MemSet(&PendingBackendStats, 0, sizeof(PgStat_BackendPending));
Not sure about this one, see above. I mean it is currently Ok but once we'll
introduce the WAL part then it will not be correct depending of the flag value
being passed.
So, I did put back the previous logic in place (setting to zero only the stats
the flush callback is responsible for) in v3 attached.
=== 4
+ * Backend statistics counts waiting to be flushed out. We assume this
variable
+ * inits to zeroes. These counters may be reported within critical sections so
+ * we use static memory in order to avoid memory allocation.
+ */
+static PgStat_BackendPending PendingBackendStats = {0};
I now think we should memset to zero to avoid any padding issues (as more
structs
will be added to PgStat_BackendPending). Oh and it's already done in
pgstat_create_backend(), so removing the {0} assignement in the attached.
=== 5
+ have_backendstats = false;
I don't think setting have_backendstats to false in pgstat_flush_backend()
is correct. I mean, it is correct currently but once we'll add the WAL part it
won't necessary be correct if the flags != PGSTAT_BACKEND_FLUSH_ALL. So, using
a
pg_memory_is_all_zeros check on PendingBackendStats instead in the attached.
> One key choice I have made is to hide PendingBackendStats within
> pgstat_backend.c so as it is possible to enforce some sanity checks
> there.
Yeah, better, thanks!
=== 6
In passing, I realized that:
"
* Copyright (c) 2001-2025, PostgreSQL Global Development Group
"
in pgstat_backend.c is incorrect (fixing it in 0002).
PFA:
0001: which is full rebase
0002: the Copyright fix
.txt: the changes I've made on top of your 0002 (to not confuse the cfbot and
to ease your review).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From c945a5800554ec90f42091d8175ed50e00ead45f Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Fri, 17 Jan 2025 06:44:47 +0000
Subject: [PATCH v3 1/2] Rework per backend pending stats
9aea73fc61d added per backend pending statistics but not all of them are well
suited to memory allocation and have to be outside of critical sections.
For those (the only current one is I/O statistics but WAL statistics is under
discussion), let's rely on a new PendingBackendStats instead.
---
src/backend/utils/activity/pgstat.c | 53 +++----
src/backend/utils/activity/pgstat_backend.c | 142 ++++++++++++------
src/backend/utils/activity/pgstat_io.c | 23 +--
src/backend/utils/activity/pgstat_relation.c | 4 +-
src/include/pgstat.h | 9 ++
src/include/utils/pgstat_internal.h | 36 +++--
.../injection_points/injection_stats.c | 2 +-
7 files changed, 153 insertions(+), 116 deletions(-)
75.8% src/backend/utils/activity/
19.6% src/include/utils/
3.6% src/include/
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 34520535d54..0a4b687f045 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -292,7 +292,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Database *) 0)->stats),
.pending_size = sizeof(PgStat_StatDBEntry),
- .flush_pending_cb = pgstat_database_flush_cb,
+ .flush_dynamic_cb = pgstat_database_flush_cb,
.reset_timestamp_cb = pgstat_database_reset_timestamp_cb,
},
@@ -307,7 +307,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Relation *) 0)->stats),
.pending_size = sizeof(PgStat_TableStatus),
- .flush_pending_cb = pgstat_relation_flush_cb,
+ .flush_dynamic_cb = pgstat_relation_flush_cb,
.delete_pending_cb = pgstat_relation_delete_pending_cb,
},
@@ -322,7 +322,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Function *) 0)->stats),
.pending_size = sizeof(PgStat_FunctionCounts),
- .flush_pending_cb = pgstat_function_flush_cb,
+ .flush_dynamic_cb = pgstat_function_flush_cb,
},
[PGSTAT_KIND_REPLSLOT] = {
@@ -355,7 +355,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Subscription *) 0)->stats),
.pending_size = sizeof(PgStat_BackendSubEntry),
- .flush_pending_cb = pgstat_subscription_flush_cb,
+ .flush_dynamic_cb = pgstat_subscription_flush_cb,
.reset_timestamp_cb = pgstat_subscription_reset_timestamp_cb,
},
@@ -372,7 +372,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Backend *) 0)->stats),
.pending_size = sizeof(PgStat_BackendPending),
- .flush_pending_cb = pgstat_backend_flush_cb,
+ .have_pending_cb = pgstat_backend_have_pending_cb,
+ .flush_static_cb = pgstat_backend_flush_cb,
.reset_timestamp_cb = pgstat_backend_reset_timestamp_cb,
},
@@ -437,8 +438,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,
+ .flush_static_cb = pgstat_io_flush_cb,
+ .have_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,8 +456,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,
+ .flush_static_cb = pgstat_slru_flush_cb,
+ .have_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,8 +475,8 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Wal *) 0)->stats),
.init_backend_cb = pgstat_wal_init_backend_cb,
- .flush_fixed_cb = pgstat_wal_flush_cb,
- .have_fixed_pending_cb = pgstat_wal_have_pending_cb,
+ .flush_static_cb = pgstat_wal_flush_cb,
+ .have_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,
@@ -713,22 +714,17 @@ pgstat_report_stat(bool force)
{
bool do_flush = false;
- /* Check for pending fixed-numbered stats */
+ /* 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->fixed_amount)
- {
- Assert(kind_info->have_fixed_pending_cb == NULL);
- continue;
- }
- if (!kind_info->have_fixed_pending_cb)
+ if (!kind_info->have_pending_cb)
continue;
- if (kind_info->have_fixed_pending_cb())
+ if (kind_info->have_pending_cb())
{
do_flush = true;
break;
@@ -789,25 +785,20 @@ pgstat_report_stat(bool force)
partial_flush = false;
- /* flush of variable-numbered stats */
+ /* flush of variable-numbered stats tracked in pending entries list */
partial_flush |= pgstat_flush_pending_entries(nowait);
- /* flush of fixed-numbered stats */
+ /* flush stats for each registered kind that has a flush static callback */
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->flush_fixed_cb == NULL);
- continue;
- }
- if (!kind_info->flush_fixed_cb)
+ if (!kind_info->flush_static_cb)
continue;
- partial_flush |= kind_info->flush_fixed_cb(nowait);
+ partial_flush |= kind_info->flush_static_cb(nowait);
}
last_flush = now;
@@ -1297,7 +1288,7 @@ pgstat_prep_pending_entry(PgStat_Kind kind, Oid dboid, uint64 objid, bool *creat
PgStat_EntryRef *entry_ref;
/* need to be able to flush out */
- Assert(pgstat_get_kind_info(kind)->flush_pending_cb != NULL);
+ Assert(pgstat_get_kind_info(kind)->flush_dynamic_cb != NULL);
if (unlikely(!pgStatPendingContext))
{
@@ -1394,10 +1385,10 @@ pgstat_flush_pending_entries(bool nowait)
dlist_node *next;
Assert(!kind_info->fixed_amount);
- Assert(kind_info->flush_pending_cb != NULL);
+ Assert(kind_info->flush_dynamic_cb != NULL);
/* flush the stats, if possible */
- did_flush = kind_info->flush_pending_cb(entry_ref, nowait);
+ did_flush = kind_info->flush_dynamic_cb(entry_ref, nowait);
Assert(did_flush || nowait);
diff --git a/src/backend/utils/activity/pgstat_backend.c b/src/backend/utils/activity/pgstat_backend.c
index 79e4d0a3053..bcae6a78169 100644
--- a/src/backend/utils/activity/pgstat_backend.c
+++ b/src/backend/utils/activity/pgstat_backend.c
@@ -11,7 +11,9 @@
* This statistics kind uses a proc number as object ID for the hash table
* of pgstats. Entries are created each time a process is spawned, and are
* dropped when the process exits. These are not written to the pgstats file
- * on disk.
+ * on disk. Pending statistics are managed without direct interactions with
+ * the pgstats dshash, relying on PendingBackendStats instead so as it is
+ * possible to report data within critical sections.
*
* Copyright (c) 2001-2025, PostgreSQL Global Development Group
*
@@ -22,8 +24,49 @@
#include "postgres.h"
+#include "storage/bufmgr.h"
+#include "utils/memutils.h"
#include "utils/pgstat_internal.h"
+/*
+ * Backend statistics counts waiting to be flushed out. These counters may be
+ * reported within critical sections so we use static memory in order to avoid
+ * memory allocation.
+ */
+static PgStat_BackendPending PendingBackendStats;
+
+/*
+ * Utility routines to report I/O stats for backends, kept here to avoid
+ * exposing PendingBackendStats to the outside world.
+ */
+void
+pgstat_count_backend_io_op_time(IOObject io_object, IOContext io_context,
+ IOOp io_op, instr_time io_time)
+{
+ Assert(track_io_timing);
+
+ if (!pgstat_tracks_backend_bktype(MyBackendType))
+ return;
+
+ Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op));
+
+ INSTR_TIME_ADD(PendingBackendStats.pending_io.pending_times[io_object][io_context][io_op],
+ io_time);
+}
+
+void
+pgstat_count_backend_io_op(IOObject io_object, IOContext io_context,
+ IOOp io_op, uint32 cnt, uint64 bytes)
+{
+ if (!pgstat_tracks_backend_bktype(MyBackendType))
+ return;
+
+ Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op));
+
+ PendingBackendStats.pending_io.counts[io_object][io_context][io_op] += cnt;
+ PendingBackendStats.pending_io.bytes[io_object][io_context][io_op] += bytes;
+}
+
/*
* Returns statistics of a backend by proc number.
*/
@@ -46,14 +89,21 @@ static void
pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_ref)
{
PgStatShared_Backend *shbackendent;
- PgStat_BackendPending *pendingent;
PgStat_BktypeIO *bktype_shstats;
- PgStat_PendingIO *pending_io;
+ PgStat_PendingIO pending_io;
+
+ /*
+ * This function can be called even if nothing at all has happened for IO
+ * statistics. In this case, avoid unnecessarily modifying the stats
+ * entry.
+ */
+ if (pg_memory_is_all_zeros(&PendingBackendStats.pending_io,
+ sizeof(struct PgStat_PendingIO)))
+ return;
shbackendent = (PgStatShared_Backend *) entry_ref->shared_stats;
- pendingent = (PgStat_BackendPending *) entry_ref->pending;
bktype_shstats = &shbackendent->stats.io_stats;
- pending_io = &pendingent->pending_io;
+ pending_io = PendingBackendStats.pending_io;
for (int io_object = 0; io_object < IOOBJECT_NUM_TYPES; io_object++)
{
@@ -64,68 +114,74 @@ pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_ref)
instr_time time;
bktype_shstats->counts[io_object][io_context][io_op] +=
- pending_io->counts[io_object][io_context][io_op];
+ pending_io.counts[io_object][io_context][io_op];
bktype_shstats->bytes[io_object][io_context][io_op] +=
- pending_io->bytes[io_object][io_context][io_op];
-
- time = pending_io->pending_times[io_object][io_context][io_op];
+ pending_io.bytes[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);
}
}
}
+
+ /*
+ * Clear out the statistics buffer, so it can be re-used.
+ */
+ MemSet(&PendingBackendStats.pending_io, 0, sizeof(PgStat_PendingIO));
}
/*
- * Wrapper routine to flush backend statistics.
+ * Flush out locally pending backend statistics
+ *
+ * "flags" parameter controls which statistics to flush. Returns true
+ * if some statistics could not be flushed.
*/
-static bool
-pgstat_flush_backend_entry(PgStat_EntryRef *entry_ref, bool nowait,
- bits32 flags)
+bool
+pgstat_flush_backend(bool nowait, bits32 flags)
{
- if (!pgstat_tracks_backend_bktype(MyBackendType))
+ PgStat_EntryRef *entry_ref;
+
+ if (pg_memory_is_all_zeros(&PendingBackendStats,
+ sizeof(struct PgStat_BackendPending)))
return false;
- if (!pgstat_lock_entry(entry_ref, nowait))
+ if (!pgstat_tracks_backend_bktype(MyBackendType))
return false;
+ entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_BACKEND, InvalidOid,
+ MyProcNumber, nowait);
+ if (!entry_ref)
+ return true;
+
/* Flush requested statistics */
if (flags & PGSTAT_BACKEND_FLUSH_IO)
pgstat_flush_backend_entry_io(entry_ref);
pgstat_unlock_entry(entry_ref);
- return true;
+ return false;
}
/*
- * Callback to flush out locally pending backend statistics.
- *
- * If no stats have been recorded, this function returns false.
+ * Check if there are any backend stats waiting for flush.
*/
bool
-pgstat_backend_flush_cb(PgStat_EntryRef *entry_ref, bool nowait)
+pgstat_backend_have_pending_cb(void)
{
- return pgstat_flush_backend_entry(entry_ref, nowait, PGSTAT_BACKEND_FLUSH_ALL);
+ return (!pg_memory_is_all_zeros(&PendingBackendStats,
+ sizeof(struct PgStat_BackendPending)));
}
/*
- * Flush out locally pending backend statistics
+ * Callback to flush out locally pending backend statistics.
*
- * "flags" parameter controls which statistics to flush.
+ * If some stats could not be flushed, return true.
*/
-void
-pgstat_flush_backend(bool nowait, bits32 flags)
+bool
+pgstat_backend_flush_cb(bool nowait)
{
- PgStat_EntryRef *entry_ref;
-
- if (!pgstat_tracks_backend_bktype(MyBackendType))
- return;
-
- entry_ref = pgstat_get_entry_ref(PGSTAT_KIND_BACKEND, InvalidOid,
- MyProcNumber, false, NULL);
- (void) pgstat_flush_backend_entry(entry_ref, nowait, flags);
+ return pgstat_flush_backend(nowait, PGSTAT_BACKEND_FLUSH_ALL);
}
/*
@@ -137,9 +193,8 @@ pgstat_create_backend(ProcNumber procnum)
PgStat_EntryRef *entry_ref;
PgStatShared_Backend *shstatent;
- entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_BACKEND, InvalidOid,
- procnum, NULL);
-
+ entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_BACKEND, InvalidOid,
+ MyProcNumber, false);
shstatent = (PgStatShared_Backend *) entry_ref->shared_stats;
/*
@@ -147,20 +202,9 @@ pgstat_create_backend(ProcNumber procnum)
* e.g. if we previously used this proc number.
*/
memset(&shstatent->stats, 0, sizeof(shstatent->stats));
-}
-
-/*
- * Find or create a local PgStat_BackendPending entry for proc number.
- */
-PgStat_BackendPending *
-pgstat_prep_backend_pending(ProcNumber procnum)
-{
- PgStat_EntryRef *entry_ref;
-
- entry_ref = pgstat_prep_pending_entry(PGSTAT_KIND_BACKEND, InvalidOid,
- procnum, NULL);
+ pgstat_unlock_entry(entry_ref);
- return entry_ref->pending;
+ MemSet(&PendingBackendStats, 0, sizeof(PgStat_BackendPending));
}
/*
diff --git a/src/backend/utils/activity/pgstat_io.c b/src/backend/utils/activity/pgstat_io.c
index 027aad8b24e..6ff5d9e96a1 100644
--- a/src/backend/utils/activity/pgstat_io.c
+++ b/src/backend/utils/activity/pgstat_io.c
@@ -73,18 +73,12 @@ pgstat_count_io_op(IOObject io_object, IOContext io_context, IOOp io_op,
Assert(pgstat_is_ioop_tracked_in_bytes(io_op) || bytes == 0);
Assert(pgstat_tracks_io_op(MyBackendType, io_object, io_context, io_op));
- if (pgstat_tracks_backend_bktype(MyBackendType))
- {
- PgStat_BackendPending *entry_ref;
-
- entry_ref = pgstat_prep_backend_pending(MyProcNumber);
- entry_ref->pending_io.counts[io_object][io_context][io_op] += cnt;
- entry_ref->pending_io.bytes[io_object][io_context][io_op] += bytes;
- }
-
PendingIOStats.counts[io_object][io_context][io_op] += cnt;
PendingIOStats.bytes[io_object][io_context][io_op] += bytes;
+ /* Add the per-backend counts */
+ pgstat_count_backend_io_op(io_object, io_context, io_op, cnt, bytes);
+
have_iostats = true;
}
@@ -145,14 +139,9 @@ pgstat_count_io_op_time(IOObject io_object, IOContext io_context, IOOp io_op,
INSTR_TIME_ADD(PendingIOStats.pending_times[io_object][io_context][io_op],
io_time);
- if (pgstat_tracks_backend_bktype(MyBackendType))
- {
- PgStat_BackendPending *entry_ref;
-
- entry_ref = pgstat_prep_backend_pending(MyProcNumber);
- INSTR_TIME_ADD(entry_ref->pending_io.pending_times[io_object][io_context][io_op],
- io_time);
- }
+ /* Add the per-backend count */
+ pgstat_count_backend_io_op_time(io_object, io_context, io_op,
+ io_time);
}
pgstat_count_io_op(io_object, io_context, io_op, cnt, bytes);
diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c
index 09247ba0971..965a7fe2c64 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_BACKEND_FLUSH_IO);
+ (void) 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_BACKEND_FLUSH_IO);
+ (void) pgstat_flush_backend(false, PGSTAT_BACKEND_FLUSH_IO);
}
/*
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 2d40fe3e70f..d0d45150977 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -540,6 +540,15 @@ extern PgStat_ArchiverStats *pgstat_fetch_stat_archiver(void);
* Functions in pgstat_backend.c
*/
+/* used by pgstat_io.c for I/O stats tracked in backends */
+extern void pgstat_count_backend_io_op_time(IOObject io_object,
+ IOContext io_context,
+ IOOp io_op,
+ instr_time io_time);
+extern void pgstat_count_backend_io_op(IOObject io_object,
+ IOContext io_context,
+ IOOp io_op, uint32 cnt,
+ uint64 bytes);
extern PgStat_Backend *pgstat_fetch_stat_backend(ProcNumber procNumber);
extern bool pgstat_tracks_backend_bktype(BackendType bktype);
extern void pgstat_create_backend(ProcNumber procnum);
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 4bb8e5c53ab..7222b414779 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -156,8 +156,8 @@ typedef struct PgStat_EntryRef
* Pending statistics data that will need to be flushed to shared memory
* stats eventually. Each stats kind utilizing pending data defines what
* format its pending data has and needs to provide a
- * PgStat_KindInfo->flush_pending_cb callback to merge pending into shared
- * stats.
+ * PgStat_KindInfo->flush_dynamic_cb callback to merge pending entries
+ * that are in dynamic memory into shared stats.
*/
void *pending;
dlist_node pending_node; /* membership in pgStatPending list */
@@ -259,10 +259,11 @@ typedef struct PgStat_KindInfo
void (*init_backend_cb) (void);
/*
- * For variable-numbered stats: flush pending stats. Required if pending
- * data is used. See flush_fixed_cb for fixed-numbered stats.
+ * For variable-numbered stats: flush pending stats entries in dynamic
+ * memory within the dshash. Required if pending data interacts with the
+ * pgstats dshash.
*/
- bool (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait);
+ bool (*flush_dynamic_cb) (PgStat_EntryRef *sr, bool nowait);
/*
* For variable-numbered stats: delete pending stats. Optional.
@@ -289,17 +290,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.
+ * For fixed-numbered or variable-numbered statistics: Check for pending
+ * stats in need of flush, when these do not use the pgstats dshash.
+ * Returns true if there are any stats pending for flush, triggering
+ * flush_cb. Optional.
*/
- bool (*flush_fixed_cb) (bool nowait);
+ bool (*have_pending_cb) (void);
/*
- * For fixed-numbered statistics: Check for pending stats in need of
- * flush. Returns true if there are any stats pending for flush. Optional.
+ * For fixed-numbered or variable-numbered statistics: Flush pending
+ * static stats. Returns true if some of the stats could not be flushed,
+ * due to lock contention for example. Optional.
*/
- bool (*have_fixed_pending_cb) (void);
+ bool (*flush_static_cb) (bool nowait);
/*
* For fixed-numbered statistics: Reset All.
@@ -617,10 +620,11 @@ extern void pgstat_archiver_snapshot_cb(void);
#define PGSTAT_BACKEND_FLUSH_IO (1 << 0) /* Flush I/O statistics */
#define PGSTAT_BACKEND_FLUSH_ALL (PGSTAT_BACKEND_FLUSH_IO)
-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);
+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);
/*
* Functions in pgstat_bgwriter.c
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index 5db62bca66f..4f3691c702b 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -48,7 +48,7 @@ static const PgStat_KindInfo injection_stats = {
.shared_data_off = offsetof(PgStatShared_InjectionPoint, stats),
.shared_data_len = sizeof(((PgStatShared_InjectionPoint *) 0)->stats),
.pending_size = sizeof(PgStat_StatInjEntry),
- .flush_pending_cb = injection_stats_flush_cb,
+ .flush_dynamic_cb = injection_stats_flush_cb,
};
/*
--
2.34.1
>From 33dbb87cd92e29a71ea2012476aca1d760c8c557 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Mon, 20 Jan 2025 10:39:50 +0000
Subject: [PATCH v3 2/2] Fixing pgstat_backend.c Copyright
The file has been introduced in 9aea73fc61d, in 2024. So fixing its Copyright
accordingly.
---
src/backend/utils/activity/pgstat_backend.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
100.0% src/backend/utils/activity/
diff --git a/src/backend/utils/activity/pgstat_backend.c b/src/backend/utils/activity/pgstat_backend.c
index bcae6a78169..b0184b3293e 100644
--- a/src/backend/utils/activity/pgstat_backend.c
+++ b/src/backend/utils/activity/pgstat_backend.c
@@ -15,7 +15,7 @@
* the pgstats dshash, relying on PendingBackendStats instead so as it is
* possible to report data within critical sections.
*
- * Copyright (c) 2001-2025, PostgreSQL Global Development Group
+ * Copyright (c) 2024-2025, PostgreSQL Global Development Group
*
* IDENTIFICATION
* src/backend/utils/activity/pgstat_backend.c
--
2.34.1
commit af786645e63b34e0d4eefbef7a8dd0794d358d0f
Author: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Mon Jan 20 09:57:26 2025 +0000
Change flush callbacks names
and the way pending backend stats are checks and set to zero.
diff --git a/src/backend/utils/activity/pgstat.c
b/src/backend/utils/activity/pgstat.c
index bd1ffad6f46..0a4b687f045 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -292,7 +292,7 @@ static const PgStat_KindInfo
pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Database *) 0)->stats),
.pending_size = sizeof(PgStat_StatDBEntry),
- .flush_pending_cb = pgstat_database_flush_cb,
+ .flush_dynamic_cb = pgstat_database_flush_cb,
.reset_timestamp_cb = pgstat_database_reset_timestamp_cb,
},
@@ -307,7 +307,7 @@ static const PgStat_KindInfo
pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Relation *) 0)->stats),
.pending_size = sizeof(PgStat_TableStatus),
- .flush_pending_cb = pgstat_relation_flush_cb,
+ .flush_dynamic_cb = pgstat_relation_flush_cb,
.delete_pending_cb = pgstat_relation_delete_pending_cb,
},
@@ -322,7 +322,7 @@ static const PgStat_KindInfo
pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Function *) 0)->stats),
.pending_size = sizeof(PgStat_FunctionCounts),
- .flush_pending_cb = pgstat_function_flush_cb,
+ .flush_dynamic_cb = pgstat_function_flush_cb,
},
[PGSTAT_KIND_REPLSLOT] = {
@@ -355,7 +355,7 @@ static const PgStat_KindInfo
pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Subscription *)
0)->stats),
.pending_size = sizeof(PgStat_BackendSubEntry),
- .flush_pending_cb = pgstat_subscription_flush_cb,
+ .flush_dynamic_cb = pgstat_subscription_flush_cb,
.reset_timestamp_cb = pgstat_subscription_reset_timestamp_cb,
},
@@ -373,7 +373,7 @@ static const PgStat_KindInfo
pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.pending_size = sizeof(PgStat_BackendPending),
.have_pending_cb = pgstat_backend_have_pending_cb,
- .flush_cb = pgstat_backend_flush_cb,
+ .flush_static_cb = pgstat_backend_flush_cb,
.reset_timestamp_cb = pgstat_backend_reset_timestamp_cb,
},
@@ -438,7 +438,7 @@ 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_cb = pgstat_io_flush_cb,
+ .flush_static_cb = pgstat_io_flush_cb,
.have_pending_cb = pgstat_io_have_pending_cb,
.init_shmem_cb = pgstat_io_init_shmem_cb,
.reset_all_cb = pgstat_io_reset_all_cb,
@@ -456,7 +456,7 @@ 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_cb = pgstat_slru_flush_cb,
+ .flush_static_cb = pgstat_slru_flush_cb,
.have_pending_cb = pgstat_slru_have_pending_cb,
.init_shmem_cb = pgstat_slru_init_shmem_cb,
.reset_all_cb = pgstat_slru_reset_all_cb,
@@ -475,7 +475,7 @@ static const PgStat_KindInfo
pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE]
.shared_data_len = sizeof(((PgStatShared_Wal *) 0)->stats),
.init_backend_cb = pgstat_wal_init_backend_cb,
- .flush_cb = pgstat_wal_flush_cb,
+ .flush_static_cb = pgstat_wal_flush_cb,
.have_pending_cb = pgstat_wal_have_pending_cb,
.init_shmem_cb = pgstat_wal_init_shmem_cb,
.reset_all_cb = pgstat_wal_reset_all_cb,
@@ -785,23 +785,20 @@ pgstat_report_stat(bool force)
partial_flush = false;
- /* flush of variable-numbered stats */
+ /* flush of variable-numbered stats tracked in pending entries list */
partial_flush |= pgstat_flush_pending_entries(nowait);
- /*
- * Flush of other stats, which could be variable-numbered or
- * fixed-numbered.
- */
+ /* flush stats for each registered kind that has a flush static
callback */
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->flush_cb)
+ if (!kind_info->flush_static_cb)
continue;
- partial_flush |= kind_info->flush_cb(nowait);
+ partial_flush |= kind_info->flush_static_cb(nowait);
}
last_flush = now;
@@ -1291,7 +1288,7 @@ pgstat_prep_pending_entry(PgStat_Kind kind, Oid dboid,
uint64 objid, bool *creat
PgStat_EntryRef *entry_ref;
/* need to be able to flush out */
- Assert(pgstat_get_kind_info(kind)->flush_pending_cb != NULL);
+ Assert(pgstat_get_kind_info(kind)->flush_dynamic_cb != NULL);
if (unlikely(!pgStatPendingContext))
{
@@ -1388,10 +1385,10 @@ pgstat_flush_pending_entries(bool nowait)
dlist_node *next;
Assert(!kind_info->fixed_amount);
- Assert(kind_info->flush_pending_cb != NULL);
+ Assert(kind_info->flush_dynamic_cb != NULL);
/* flush the stats, if possible */
- did_flush = kind_info->flush_pending_cb(entry_ref, nowait);
+ did_flush = kind_info->flush_dynamic_cb(entry_ref, nowait);
Assert(did_flush || nowait);
diff --git a/src/backend/utils/activity/pgstat_backend.c
b/src/backend/utils/activity/pgstat_backend.c
index fb7abf64a0b..bcae6a78169 100644
--- a/src/backend/utils/activity/pgstat_backend.c
+++ b/src/backend/utils/activity/pgstat_backend.c
@@ -29,12 +29,11 @@
#include "utils/pgstat_internal.h"
/*
- * Backend statistics counts waiting to be flushed out. We assume this
variable
- * inits to zeroes. These counters may be reported within critical sections so
- * we use static memory in order to avoid memory allocation.
+ * Backend statistics counts waiting to be flushed out. These counters may be
+ * reported within critical sections so we use static memory in order to avoid
+ * memory allocation.
*/
-static PgStat_BackendPending PendingBackendStats = {0};
-static bool have_backendstats = false;
+static PgStat_BackendPending PendingBackendStats;
/*
* Utility routines to report I/O stats for backends, kept here to avoid
@@ -53,8 +52,6 @@ pgstat_count_backend_io_op_time(IOObject io_object, IOContext
io_context,
INSTR_TIME_ADD(PendingBackendStats.pending_io.pending_times[io_object][io_context][io_op],
io_time);
-
- have_backendstats = true;
}
void
@@ -68,8 +65,6 @@ pgstat_count_backend_io_op(IOObject io_object, IOContext
io_context,
PendingBackendStats.pending_io.counts[io_object][io_context][io_op] +=
cnt;
PendingBackendStats.pending_io.bytes[io_object][io_context][io_op] +=
bytes;
-
- have_backendstats = true;
}
/*
@@ -98,8 +93,8 @@ pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_ref)
PgStat_PendingIO pending_io;
/*
- * This function can be called even if nothing at all has happened for
- * IO statistics. In this case, avoid unnecessarily modifying the stats
+ * This function can be called even if nothing at all has happened for
IO
+ * statistics. In this case, avoid unnecessarily modifying the stats
* entry.
*/
if (pg_memory_is_all_zeros(&PendingBackendStats.pending_io,
@@ -129,6 +124,11 @@ pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_ref)
}
}
}
+
+ /*
+ * Clear out the statistics buffer, so it can be re-used.
+ */
+ MemSet(&PendingBackendStats.pending_io, 0, sizeof(PgStat_PendingIO));
}
/*
@@ -142,7 +142,8 @@ pgstat_flush_backend(bool nowait, bits32 flags)
{
PgStat_EntryRef *entry_ref;
- if (!have_backendstats)
+ if (pg_memory_is_all_zeros(&PendingBackendStats,
+ sizeof(struct
PgStat_BackendPending)))
return false;
if (!pgstat_tracks_backend_bktype(MyBackendType))
@@ -159,12 +160,6 @@ pgstat_flush_backend(bool nowait, bits32 flags)
pgstat_unlock_entry(entry_ref);
- /*
- * Clear out the statistics buffer, so it can be re-used.
- */
- MemSet(&PendingBackendStats, 0, sizeof(PgStat_BackendPending));
-
- have_backendstats = false;
return false;
}
@@ -174,7 +169,8 @@ pgstat_flush_backend(bool nowait, bits32 flags)
bool
pgstat_backend_have_pending_cb(void)
{
- return have_backendstats;
+ return (!pg_memory_is_all_zeros(&PendingBackendStats,
+
sizeof(struct PgStat_BackendPending)));
}
/*
@@ -209,7 +205,6 @@ pgstat_create_backend(ProcNumber procnum)
pgstat_unlock_entry(entry_ref);
MemSet(&PendingBackendStats, 0, sizeof(PgStat_BackendPending));
- have_backendstats = false;
}
/*
diff --git a/src/include/utils/pgstat_internal.h
b/src/include/utils/pgstat_internal.h
index 000ed5b36f6..7222b414779 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -156,8 +156,8 @@ typedef struct PgStat_EntryRef
* Pending statistics data that will need to be flushed to shared memory
* stats eventually. Each stats kind utilizing pending data defines what
* format its pending data has and needs to provide a
- * PgStat_KindInfo->flush_pending_cb callback to merge pending into
shared
- * stats.
+ * PgStat_KindInfo->flush_dynamic_cb callback to merge pending entries
+ * that are in dynamic memory into shared stats.
*/
void *pending;
dlist_node pending_node; /* membership in pgStatPending list */
@@ -259,10 +259,11 @@ typedef struct PgStat_KindInfo
void (*init_backend_cb) (void);
/*
- * For variable-numbered stats: flush pending stats within the dshash.
- * Required if pending data interacts with the pgstats dshash.
+ * For variable-numbered stats: flush pending stats entries in dynamic
+ * memory within the dshash. Required if pending data interacts with the
+ * pgstats dshash.
*/
- bool (*flush_pending_cb) (PgStat_EntryRef *sr, bool nowait);
+ bool (*flush_dynamic_cb) (PgStat_EntryRef *sr, bool nowait);
/*
* For variable-numbered stats: delete pending stats. Optional.
@@ -298,10 +299,10 @@ typedef struct PgStat_KindInfo
/*
* For fixed-numbered or variable-numbered statistics: Flush pending
- * stats. Returns true if some of the stats could not be flushed, due
- * to lock contention for example. Optional.
+ * static stats. Returns true if some of the stats could not be flushed,
+ * due to lock contention for example. Optional.
*/
- bool (*flush_cb) (bool nowait);
+ bool (*flush_static_cb) (bool nowait);
/*
* For fixed-numbered statistics: Reset All.
diff --git a/src/test/modules/injection_points/injection_stats.c
b/src/test/modules/injection_points/injection_stats.c
index 5db62bca66f..4f3691c702b 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -48,7 +48,7 @@ static const PgStat_KindInfo injection_stats = {
.shared_data_off = offsetof(PgStatShared_InjectionPoint, stats),
.shared_data_len = sizeof(((PgStatShared_InjectionPoint *) 0)->stats),
.pending_size = sizeof(PgStat_StatInjEntry),
- .flush_pending_cb = injection_stats_flush_cb,
+ .flush_dynamic_cb = injection_stats_flush_cb,
};
/*
