At Thu, 6 Oct 2022 13:44:43 +0900, Michael Paquier <mich...@paquier.xyz> wrote
in
> On Wed, Oct 05, 2022 at 11:24:57PM -0400, Jonathan S. Katz wrote:
> > On 10/5/22 8:44 PM, Andres Freund wrote:
> >> I have two ideas how to fix it. As a design constraint, I'd be interested
> >> in
> >> the RMTs opinion on the following:
> >> Is a cleaner fix that changes the stats format (i.e. existing stats will be
> >> thrown away when upgrading) or one that doesn't change the stats format
> >> preferrable?
> >
> > [My opinion, will let Michael/John chime in]
> >
> > Ideally we would keep the stats on upgrade -- I think that's the better user
> > experience.
>
> The release has not happened yet, so I would be fine to remain
> flexible and bump again PGSTAT_FILE_FORMAT_ID so as we have the
> cleanest approach in place for the release and the future. At the
> end, we would throw automatically the data of a file that's marked
> with a version that does not match with what we expect at load time,
> so there's a limited impact on the user experience, except, well,
> losing these stats, of course.
+1. FWIW, the atttached is an example of what it looks like if we
avoid file format change.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 4effe9279d6535191d7f5478bb76490dc8762d33 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Thu, 6 Oct 2022 14:06:04 +0900
Subject: [PATCH v2] If we avoid file format
---
src/backend/utils/activity/pgstat.c | 9 +++++++--
src/backend/utils/activity/pgstat_replslot.c | 18 ++++++++++++------
src/include/pgstat.h | 1 -
src/include/utils/pgstat_internal.h | 3 +++
4 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 609f0b1ad8..a03184c9ef 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -310,6 +310,7 @@ static const PgStat_KindInfo pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
.reset_timestamp_cb = pgstat_replslot_reset_timestamp_cb,
.to_serialized_name = pgstat_replslot_to_serialized_name_cb,
+ .set_name = pgstat_replslot_set_name_cb,
.from_serialized_name = pgstat_replslot_from_serialized_name_cb,
},
@@ -1523,6 +1524,8 @@ pgstat_read_statsfile(void)
PgStat_HashKey key;
PgStatShared_HashEntry *p;
PgStatShared_Common *header;
+ const PgStat_KindInfo *kind_info = NULL;
+ NameData name = {0};
CHECK_FOR_INTERRUPTS();
@@ -1538,9 +1541,7 @@ pgstat_read_statsfile(void)
else
{
/* stats entry identified by name on disk (e.g. slots) */
- const PgStat_KindInfo *kind_info = NULL;
PgStat_Kind kind;
- NameData name;
if (!read_chunk_s(fpin, &kind))
goto error;
@@ -1590,6 +1591,10 @@ pgstat_read_statsfile(void)
pgstat_get_entry_len(key.kind)))
goto error;
+ /* set name if it requires to do that separately */
+ if (kind_info && kind_info->set_name && NameStr(name)[0])
+ kind_info->set_name(header, &name);
+
break;
}
case 'E':
diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/utils/activity/pgstat_replslot.c
index 2039ac3147..9fa4df610d 100644
--- a/src/backend/utils/activity/pgstat_replslot.c
+++ b/src/backend/utils/activity/pgstat_replslot.c
@@ -86,7 +86,7 @@ pgstat_report_replslot(ReplicationSlot *slot, const PgStat_StatReplSlotEntry *re
* Any mismatch should have been fixed in pgstat_create_replslot() or
* pgstat_acquire_replslot().
*/
- Assert(namestrcmp(&statent->slotname, NameStr(slot->data.name)) == 0);
+ Assert(namestrcmp(&shstatent->slotname, NameStr(slot->data.name)) == 0);
/* Update the replication slot statistics */
#define REPLSLOT_ACC(fld) statent->fld += repSlotStat->fld
@@ -124,7 +124,7 @@ pgstat_create_replslot(ReplicationSlot *slot)
* if we previously crashed after dropping a slot.
*/
memset(&shstatent->stats, 0, sizeof(shstatent->stats));
- namestrcpy(&shstatent->stats.slotname, NameStr(slot->data.name));
+ namestrcpy(&shstatent->slotname, NameStr(slot->data.name));
pgstat_unlock_entry(entry_ref);
}
@@ -148,11 +148,11 @@ pgstat_acquire_replslot(ReplicationSlot *slot)
* NB: need to accept that there might be stats from an older slot, e.g.
* if we previously crashed after dropping a slot.
*/
- if (NameStr(statent->slotname)[0] == 0 ||
- namestrcmp(&statent->slotname, NameStr(slot->data.name)) != 0)
+ if (NameStr(shstatent->slotname)[0] == 0 ||
+ namestrcmp(&shstatent->slotname, NameStr(slot->data.name)) != 0)
{
memset(statent, 0, sizeof(*statent));
- namestrcpy(&statent->slotname, NameStr(slot->data.name));
+ namestrcpy(&shstatent->slotname, NameStr(slot->data.name));
}
pgstat_unlock_entry(entry_ref);
@@ -187,7 +187,13 @@ pgstat_fetch_replslot(NameData slotname)
void
pgstat_replslot_to_serialized_name_cb(const PgStatShared_Common *header, NameData *name)
{
- namestrcpy(name, NameStr(((PgStatShared_ReplSlot *) header)->stats.slotname));
+ namestrcpy(name, NameStr(((PgStatShared_ReplSlot *) header)->slotname));
+}
+
+void
+pgstat_replslot_set_name_cb(const PgStatShared_Common *header, NameData *name)
+{
+ namestrcpy(&((PgStatShared_ReplSlot *) header)->slotname, NameStr(*name));
}
bool
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index ad7334a0d2..530d49e5c3 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -321,7 +321,6 @@ typedef struct PgStat_StatFuncEntry
typedef struct PgStat_StatReplSlotEntry
{
- NameData slotname;
PgStat_Counter spill_txns;
PgStat_Counter spill_count;
PgStat_Counter spill_bytes;
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 40a3602855..ff07247062 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -244,6 +244,7 @@ typedef struct PgStat_KindInfo
*/
void (*to_serialized_name) (const PgStatShared_Common *header, NameData *name);
bool (*from_serialized_name) (const NameData *name, PgStat_HashKey *key);
+ void (*set_name) (const PgStatShared_Common *header, NameData *name);
/*
* For fixed-numbered statistics: Reset All.
@@ -380,6 +381,7 @@ typedef struct PgStatShared_Subscription
typedef struct PgStatShared_ReplSlot
{
PgStatShared_Common header;
+ NameData slotname;
PgStat_StatReplSlotEntry stats;
} PgStatShared_ReplSlot;
@@ -568,6 +570,7 @@ extern void pgstat_relation_delete_pending_cb(PgStat_EntryRef *entry_ref);
extern void pgstat_replslot_reset_timestamp_cb(PgStatShared_Common *header, TimestampTz ts);
extern void pgstat_replslot_to_serialized_name_cb(const PgStatShared_Common *header, NameData *name);
+extern void pgstat_replslot_set_name_cb(const PgStatShared_Common *header, NameData *name);
extern bool pgstat_replslot_from_serialized_name_cb(const NameData *name, PgStat_HashKey *key);
--
2.31.1
