Hi, On 2022-10-06 14:10:46 +0900, Kyotaro Horiguchi wrote: > +1. FWIW, the atttached is an example of what it looks like if we > avoid file format change.
What about if we go the other direction - simply remove the name from the stats entry at all. I don't actually think we need it anymore. Unless I am missing something right now - entirely possible! - the danger that pgstat_acquire_replslot() mentions doesn't actually exist [anymore]. 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. The attached is a rough prototype, but should be enough for Jaime to test and Horiguchi to test / review / think about. Amit, I CCed you, since you've thought a bunch about the dangers in this area too. Greetings, Andres Freund
>From f34c6ed510fbe4c079a0c80e9040cac77c60fd19 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@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
