From: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> |
---|---|
To: | andres(at)anarazel(dot)de |
Cc: | jcasanov(at)systemguards(dot)com(dot)ec, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: START_REPLICATION SLOT causing a crash in an assert build |
Date: | 2022-09-27 05:52:38 |
Message-ID: | 20220927.145238.1330141994586078168.horikyota.ntt@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks!
At Mon, 26 Sep 2022 19:53:02 -0700, Andres Freund <andres(at)anarazel(dot)de> 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
Attachment | Content-Type | Size |
---|---|---|
v1-0001-Do-not-reset-slot-name-of-replication-slot-stats.patch | text/x-patch | 3.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Bharath Rupireddy | 2022-09-27 06:03:56 | Re: Avoid memory leaks during base backups |
Previous Message | Bharath Rupireddy | 2022-09-27 05:42:59 | Re: Use pg_pwritev_with_retry() instead of write() in dir_open_for_write() to avoid partial writes? |