From f34c6ed510fbe4c079a0c80e9040cac77c60fd19 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 6 Oct 2022 15:50:36 -0700
Subject: [PATCH v3] pgstat: Remove slotname from PgStat_StatReplSlotEntry

This fixes a bug where the slotname is reset as part of
pgstat_reset_replslot() subsequently causing an allocation failure in
pgstat_report_replslot(). It also is architecturally nicer, because having the
name as part of the stats isn't quite right, on account of not actually being
stats :)

As far as I can tell we actually have worked around all the dangers that lead
us to keeping the stats name part of the stats.

Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/include/pgstat.h                         |  2 ++
 src/include/utils/pgstat_internal.h          |  5 +--
 src/backend/utils/activity/pgstat.c          |  2 +-
 src/backend/utils/activity/pgstat_replslot.c | 36 +++++---------------
 4 files changed, 14 insertions(+), 31 deletions(-)

diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index ad7334a0d21..2a85fa0369a 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -321,7 +321,9 @@ typedef struct PgStat_StatFuncEntry
 
 typedef struct PgStat_StatReplSlotEntry
 {
+#if IN_PG_15_ONLY_UNUSED
 	NameData	slotname;
+#endif
 	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 40a36028554..627c1389e4c 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -242,7 +242,8 @@ typedef struct PgStat_KindInfo
 	/*
 	 * For variable-numbered stats with named_on_disk. Optional.
 	 */
-	void		(*to_serialized_name) (const PgStatShared_Common *header, NameData *name);
+	void		(*to_serialized_name) (const PgStat_HashKey *key,
+									   const PgStatShared_Common *header, NameData *name);
 	bool		(*from_serialized_name) (const NameData *name, PgStat_HashKey *key);
 
 	/*
@@ -567,7 +568,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_to_serialized_name_cb(const PgStat_HashKey *key, const PgStatShared_Common *header, NameData *name);
 extern bool pgstat_replslot_from_serialized_name_cb(const NameData *name, PgStat_HashKey *key);
 
 
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 609f0b1ad86..1b97597f17c 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1367,7 +1367,7 @@ pgstat_write_statsfile(void)
 			/* stats entry identified by name on disk (e.g. slots) */
 			NameData	name;
 
-			kind_info->to_serialized_name(shstats, &name);
+			kind_info->to_serialized_name(&ps->key, shstats, &name);
 
 			fputc('N', fpout);
 			write_chunk_s(fpout, &ps->key.kind);
diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/utils/activity/pgstat_replslot.c
index 2039ac3147f..793f26fc950 100644
--- a/src/backend/utils/activity/pgstat_replslot.c
+++ b/src/backend/utils/activity/pgstat_replslot.c
@@ -82,12 +82,6 @@ pgstat_report_replslot(ReplicationSlot *slot, const PgStat_StatReplSlotEntry *re
 	shstatent = (PgStatShared_ReplSlot *) entry_ref->shared_stats;
 	statent = &shstatent->stats;
 
-	/*
-	 * Any mismatch should have been fixed in pgstat_create_replslot() or
-	 * pgstat_acquire_replslot().
-	 */
-	Assert(namestrcmp(&statent->slotname, NameStr(slot->data.name)) == 0);
-
 	/* Update the replication slot statistics */
 #define REPLSLOT_ACC(fld) statent->fld += repSlotStat->fld
 	REPLSLOT_ACC(spill_txns);
@@ -124,7 +118,6 @@ 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));
 
 	pgstat_unlock_entry(entry_ref);
 }
@@ -135,27 +128,13 @@ pgstat_create_replslot(ReplicationSlot *slot)
 void
 pgstat_acquire_replslot(ReplicationSlot *slot)
 {
-	PgStat_EntryRef *entry_ref;
-	PgStatShared_ReplSlot *shstatent;
-	PgStat_StatReplSlotEntry *statent;
-
-	entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_REPLSLOT, InvalidOid,
-											ReplicationSlotIndex(slot), false);
-	shstatent = (PgStatShared_ReplSlot *) entry_ref->shared_stats;
-	statent = &shstatent->stats;
-
 	/*
-	 * NB: need to accept that there might be stats from an older slot, e.g.
-	 * if we previously crashed after dropping a slot.
+	 * XXX: I think there cannot actually be data from an older slot
+	 * here. After a crash we throw away the old stats data and if a slot is
+	 * dropped while shut down, we'll not load the slot data at startup.
 	 */
-	if (NameStr(statent->slotname)[0] == 0 ||
-		namestrcmp(&statent->slotname, NameStr(slot->data.name)) != 0)
-	{
-		memset(statent, 0, sizeof(*statent));
-		namestrcpy(&statent->slotname, NameStr(slot->data.name));
-	}
-
-	pgstat_unlock_entry(entry_ref);
+	pgstat_get_entry_ref(PGSTAT_KIND_REPLSLOT, InvalidOid,
+						 ReplicationSlotIndex(slot), false, NULL);
 }
 
 /*
@@ -185,9 +164,10 @@ pgstat_fetch_replslot(NameData slotname)
 }
 
 void
-pgstat_replslot_to_serialized_name_cb(const PgStatShared_Common *header, NameData *name)
+pgstat_replslot_to_serialized_name_cb(const PgStat_HashKey *key, const PgStatShared_Common *header, NameData *name)
 {
-	namestrcpy(name, NameStr(((PgStatShared_ReplSlot *) header)->stats.slotname));
+	/* FIXME: wrap in a slot.c function */
+	namestrcpy(name, NameStr(ReplicationSlotCtl->replication_slots[key->objoid].data.name));
 }
 
 bool
-- 
2.38.0

