Thanks!
At Mon, 26 Sep 2022 19:53:02 -0700, Andres Freund <[email protected]> wrote in
> I wonder if the correct fix here wouldn't be to move the slotname out of
> PgStat_StatReplSlotEntry?
Ugh. Right. I thought its outer struct as purely the part for the
common header. But we can freely place anything after the header
part. I moved it to the outer struct. I didn't clear that part in
pgstat_create_relation() because it is filled in immediately.
The attached is that.
> On 2022-09-16 14:37:17 +0900, Kyotaro Horiguchi wrote:
...
> > @@ -307,6 +313,10 @@ static const PgStat_KindInfo
> > pgstat_kind_infos[PGSTAT_NUM_KINDS] = {
> > .shared_size = sizeof(PgStatShared_ReplSlot),
> > .shared_data_off = offsetof(PgStatShared_ReplSlot, stats),
> > .shared_data_len = sizeof(((PgStatShared_ReplSlot *) 0)->stats),
> > + /* reset doesn't wipe off slot name */
> > + .reset_off = offsetof(PgStat_StatReplSlotEntry, spill_txns),
> > + .reset_len = sizeof(((PgStatShared_ReplSlot *) 0)->stats),
> > + offsetof(PgStat_StatReplSlotEntry, spill_txns),
>
> I'm confused what this offsetof does here? It's not even assigned to a
> specific field? Am I missing something?
>
> Also, wouldn't we need to subtract something of the size?
Yeah, I felt it confusing. The last line above is offset from just
after the header part (it is PgStat_, not PgStatShared_). I first
wrote that as you suggested but rewrote to shorter representation.
>
> > diff --git a/src/backend/utils/activity/pgstat_shmem.c
> > b/src/backend/utils/activity/pgstat_shmem.c
> > index ac98918688..09a8c3873c 100644
> > --- a/src/backend/utils/activity/pgstat_shmem.c
> > +++ b/src/backend/utils/activity/pgstat_shmem.c
> > @@ -915,8 +915,9 @@ shared_stat_reset_contents(PgStat_Kind kind,
> > PgStatShared_Common *header,
> > {
> > const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
> >
> > - memset(pgstat_get_entry_data(kind, header), 0,
> > - pgstat_get_entry_len(kind));
> > + memset((char *)pgstat_get_entry_data(kind, header) +
> > + kind_info->reset_off, 0,
> > + kind_info->reset_len);
> >
> > if (kind_info->reset_timestamp_cb)
> > kind_info->reset_timestamp_cb(header, ts);
>
> This likely doesn't quite conform to what pgindent wants...
In the first place, it's ugly...
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 5789b35c1eca77ad63066e6671921e1e8a656372 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <[email protected]>
Date: Tue, 27 Sep 2022 14:28:56 +0900
Subject: [PATCH v1] Do not reset slot name of replication slot stats
pg_stat_reset_replication_slot() clears counters involving slot name,
which is not intended and causes crash. Move slot name out of
PgStat_StatReplSlotEntry to PgStatShared_Database so that the name
won't be wiped out by a counter reset.
---
src/backend/utils/activity/pgstat_replslot.c | 12 ++++++------
src/include/pgstat.h | 1 -
src/include/utils/pgstat_internal.h | 1 +
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/utils/activity/pgstat_replslot.c
index 2039ac3147..0a383f9f32 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,7 @@ 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));
}
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..9bdcc08ed3 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -380,6 +380,7 @@ typedef struct PgStatShared_Subscription
typedef struct PgStatShared_ReplSlot
{
PgStatShared_Common header;
+ NameData slotname;
PgStat_StatReplSlotEntry stats;
} PgStatShared_ReplSlot;
--
2.31.1
