Hi,
On Tue, Jul 09, 2024 at 10:45:05AM +0900, Michael Paquier wrote:
> On Mon, Jul 08, 2024 at 07:22:32AM +0000, Bertrand Drouvot wrote:
> > Yeah, what I meant to say is that one could think for example that's the
> > PgStatShared_Archiver size while in fact it's the PgStat_ArchiverStats size.
> > I think it's more confusing when writing the stats. Here we are manipulating
> > "snapshot" and "snapshot" offsets. It was not that confusing when reading
> > as we
> > are manipulating "shmem" and "shared" offsets.
> >
> > As I said, the code is fully correct, that's just the wording here that
> > sounds
> > weird to me in the "snapshot" context.
>
> After sleeping on it, I can see your point. If we were to do the
> (shared_data_len -> stats_data_len) switch, could it make sense to
> rename shared_data_off to stats_data_off to have a better symmetry?
> This one is the offset of the stats data in a shmem entry, so perhaps
> shared_data_off is OK, but it feels a bit inconsistent as well.
Agree that if we were to rename one of them then the second one should be
renamed to.
I gave a second thought on it, and I think that this is the "data" part that
lead
to the confusion (as too generic), what about?
shared_data_len -> shared_stats_len
shared_data_off -> shared_stats_off
That looks ok to me even in the snapshot context (shared is fine after all
because that's where the stats come from).
Attached a patch proposal doing so.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From 390fdbc5b3fb1f25df809d0541507b886f1b15ec Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Tue, 9 Jul 2024 04:52:04 +0000
Subject: [PATCH v1] Renaming two fields in PgStat_KindInfo
Renaming shared_data_off to shared_stats_off and shared_data_len to
shared_stats_len. "data" seems too generic and started to be confusing since
b68b29bc8f makes use of the "_len" one in the snapshot context. Using "stats"
instead as that's what those fields are linked to.
---
src/backend/utils/activity/pgstat.c | 56 ++++++++++++++---------------
src/include/utils/pgstat_internal.h | 12 +++----
2 files changed, 34 insertions(+), 34 deletions(-)
84.2% src/backend/utils/activity/
15.7% src/include/utils/
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 65b937a85f..bb777423fb 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -274,8 +274,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.accessed_across_databases = true,
.shared_size = sizeof(PgStatShared_Database),
- .shared_data_off = offsetof(PgStatShared_Database, stats),
- .shared_data_len = sizeof(((PgStatShared_Database *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_Database, stats),
+ .shared_stats_len = sizeof(((PgStatShared_Database *) 0)->stats),
.pending_size = sizeof(PgStat_StatDBEntry),
.flush_pending_cb = pgstat_database_flush_cb,
@@ -288,8 +288,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.fixed_amount = false,
.shared_size = sizeof(PgStatShared_Relation),
- .shared_data_off = offsetof(PgStatShared_Relation, stats),
- .shared_data_len = sizeof(((PgStatShared_Relation *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_Relation, stats),
+ .shared_stats_len = sizeof(((PgStatShared_Relation *) 0)->stats),
.pending_size = sizeof(PgStat_TableStatus),
.flush_pending_cb = pgstat_relation_flush_cb,
@@ -302,8 +302,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.fixed_amount = false,
.shared_size = sizeof(PgStatShared_Function),
- .shared_data_off = offsetof(PgStatShared_Function, stats),
- .shared_data_len = sizeof(((PgStatShared_Function *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_Function, stats),
+ .shared_stats_len = sizeof(((PgStatShared_Function *) 0)->stats),
.pending_size = sizeof(PgStat_FunctionCounts),
.flush_pending_cb = pgstat_function_flush_cb,
@@ -317,8 +317,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.accessed_across_databases = true,
.shared_size = sizeof(PgStatShared_ReplSlot),
- .shared_data_off = offsetof(PgStatShared_ReplSlot, stats),
- .shared_data_len = sizeof(((PgStatShared_ReplSlot *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_ReplSlot, stats),
+ .shared_stats_len = sizeof(((PgStatShared_ReplSlot *) 0)->stats),
.reset_timestamp_cb = pgstat_replslot_reset_timestamp_cb,
.to_serialized_name = pgstat_replslot_to_serialized_name_cb,
@@ -333,8 +333,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.accessed_across_databases = true,
.shared_size = sizeof(PgStatShared_Subscription),
- .shared_data_off = offsetof(PgStatShared_Subscription, stats),
- .shared_data_len = sizeof(((PgStatShared_Subscription *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_Subscription, stats),
+ .shared_stats_len = sizeof(((PgStatShared_Subscription *) 0)->stats),
.pending_size = sizeof(PgStat_BackendSubEntry),
.flush_pending_cb = pgstat_subscription_flush_cb,
@@ -351,8 +351,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.snapshot_ctl_off = offsetof(PgStat_Snapshot, archiver),
.shared_ctl_off = offsetof(PgStat_ShmemControl, archiver),
- .shared_data_off = offsetof(PgStatShared_Archiver, stats),
- .shared_data_len = sizeof(((PgStatShared_Archiver *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_Archiver, stats),
+ .shared_stats_len = sizeof(((PgStatShared_Archiver *) 0)->stats),
.reset_all_cb = pgstat_archiver_reset_all_cb,
.snapshot_cb = pgstat_archiver_snapshot_cb,
@@ -365,8 +365,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.snapshot_ctl_off = offsetof(PgStat_Snapshot, bgwriter),
.shared_ctl_off = offsetof(PgStat_ShmemControl, bgwriter),
- .shared_data_off = offsetof(PgStatShared_BgWriter, stats),
- .shared_data_len = sizeof(((PgStatShared_BgWriter *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_BgWriter, stats),
+ .shared_stats_len = sizeof(((PgStatShared_BgWriter *) 0)->stats),
.reset_all_cb = pgstat_bgwriter_reset_all_cb,
.snapshot_cb = pgstat_bgwriter_snapshot_cb,
@@ -379,8 +379,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.snapshot_ctl_off = offsetof(PgStat_Snapshot, checkpointer),
.shared_ctl_off = offsetof(PgStat_ShmemControl, checkpointer),
- .shared_data_off = offsetof(PgStatShared_Checkpointer, stats),
- .shared_data_len = sizeof(((PgStatShared_Checkpointer *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_Checkpointer, stats),
+ .shared_stats_len = sizeof(((PgStatShared_Checkpointer *) 0)->stats),
.reset_all_cb = pgstat_checkpointer_reset_all_cb,
.snapshot_cb = pgstat_checkpointer_snapshot_cb,
@@ -393,8 +393,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.snapshot_ctl_off = offsetof(PgStat_Snapshot, io),
.shared_ctl_off = offsetof(PgStat_ShmemControl, io),
- .shared_data_off = offsetof(PgStatShared_IO, stats),
- .shared_data_len = sizeof(((PgStatShared_IO *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_IO, stats),
+ .shared_stats_len = sizeof(((PgStatShared_IO *) 0)->stats),
.reset_all_cb = pgstat_io_reset_all_cb,
.snapshot_cb = pgstat_io_snapshot_cb,
@@ -407,8 +407,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.snapshot_ctl_off = offsetof(PgStat_Snapshot, slru),
.shared_ctl_off = offsetof(PgStat_ShmemControl, slru),
- .shared_data_off = offsetof(PgStatShared_SLRU, stats),
- .shared_data_len = sizeof(((PgStatShared_SLRU *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_SLRU, stats),
+ .shared_stats_len = sizeof(((PgStatShared_SLRU *) 0)->stats),
.reset_all_cb = pgstat_slru_reset_all_cb,
.snapshot_cb = pgstat_slru_snapshot_cb,
@@ -421,8 +421,8 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.snapshot_ctl_off = offsetof(PgStat_Snapshot, wal),
.shared_ctl_off = offsetof(PgStat_ShmemControl, wal),
- .shared_data_off = offsetof(PgStatShared_Wal, stats),
- .shared_data_len = sizeof(((PgStatShared_Wal *) 0)->stats),
+ .shared_stats_off = offsetof(PgStatShared_Wal, stats),
+ .shared_stats_len = sizeof(((PgStatShared_Wal *) 0)->stats),
.reset_all_cb = pgstat_wal_reset_all_cb,
.snapshot_cb = pgstat_wal_snapshot_cb,
@@ -910,15 +910,15 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
* repeated accesses.
*/
if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_NONE)
- stats_data = palloc(kind_info->shared_data_len);
+ stats_data = palloc(kind_info->shared_stats_len);
else
stats_data = MemoryContextAlloc(pgStatLocal.snapshot.context,
- kind_info->shared_data_len);
+ kind_info->shared_stats_len);
pgstat_lock_entry_shared(entry_ref, false);
memcpy(stats_data,
pgstat_get_entry_data(kind, entry_ref->shared_stats),
- kind_info->shared_data_len);
+ kind_info->shared_stats_len);
pgstat_unlock_entry(entry_ref);
if (pgstat_fetch_consistency > PGSTAT_FETCH_CONSISTENCY_NONE)
@@ -1390,7 +1390,7 @@ pgstat_write_statsfile(void)
pgstat_build_snapshot_fixed(kind);
ptr = ((char *) &pgStatLocal.snapshot) + info->snapshot_ctl_off;
- write_chunk(fpout, ptr, info->shared_data_len);
+ write_chunk(fpout, ptr, info->shared_stats_len);
}
/*
@@ -1542,8 +1542,8 @@ pgstat_read_statsfile(void)
Assert(info->shared_ctl_off != 0);
- ptr = ((char *) shmem) + info->shared_ctl_off + info->shared_data_off;
- if (!read_chunk(fpin, ptr, info->shared_data_len))
+ ptr = ((char *) shmem) + info->shared_ctl_off + info->shared_stats_off;
+ if (!read_chunk(fpin, ptr, info->shared_stats_len))
goto error;
}
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 43d6deb03c..9246998ac6 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -217,8 +217,8 @@ typedef struct PgStat_KindInfo
* shared_size because [de-]serialization may not include in-memory state
* like lwlocks.
*/
- uint32 shared_data_off;
- uint32 shared_data_len;
+ uint32 shared_stats_off;
+ uint32 shared_stats_len;
/*
* The size of the pending data for this kind. E.g. how large
@@ -789,22 +789,22 @@ pgstat_hash_hash_key(const void *d, size_t size, void *arg)
}
/*
- * The length of the data portion of a shared memory stats entry (i.e. without
+ * The length of the stats portion of a shared memory stats entry (i.e. without
* transient data such as refcounts, lwlocks, ...).
*/
static inline size_t
pgstat_get_entry_len(PgStat_Kind kind)
{
- return pgstat_get_kind_info(kind)->shared_data_len;
+ return pgstat_get_kind_info(kind)->shared_stats_len;
}
/*
- * Returns a pointer to the data portion of a shared memory stats entry.
+ * Returns a pointer to the stats portion of a shared memory stats entry.
*/
static inline void *
pgstat_get_entry_data(PgStat_Kind kind, PgStatShared_Common *entry)
{
- size_t off = pgstat_get_kind_info(kind)->shared_data_off;
+ size_t off = pgstat_get_kind_info(kind)->shared_stats_off;
Assert(off != 0 && off < PG_UINT32_MAX);
--
2.34.1
