Re: Replication slot stats misgivings

On Wed, Apr 21, 2021 at 6:20 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Apr 21, 2021 at 2:37 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Wed, Apr 21, 2021 at 12:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Apr 20, 2021 at 7:54 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > >
> > >
> > > I have one question:
> > >
> > > + /*
> > > + * Create the replication slot stats hash table if we don't have
> > > + * it already.
> > > + */
> > > + if (replSlotStats == NULL)
> > > {
> > > - if (namestrcmp(&replSlotStats[i].slotname, name) == 0)
> > > - return i; /* found */
> > > + HASHCTL hash_ctl;
> > > +
> > > + hash_ctl.keysize = sizeof(NameData);
> > > + hash_ctl.entrysize = sizeof(PgStat_StatReplSlotEntry);
> > > + hash_ctl.hcxt = pgStatLocalContext;
> > > +
> > > + replSlotStats = hash_create("Replication slots hash",
> > > + PGSTAT_REPLSLOT_HASH_SIZE,
> > > + &hash_ctl,
> > > + HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
> > > }
> > >
> > > It seems to me that the patch is always creating a hash table in
> > > pgStatLocalContext? AFAIU, we need to create it in pgStatLocalContext
> > > when we read stats via backend_read_statsfile so that we can clear it
> > > at the end of the transaction. The db/function stats seems to be doing
> > > the same. Is there a reason why here we need to always create it in
> > > pgStatLocalContext?
> >
> > I wanted to avoid creating the hash table if there is no replication
> > slot. But as you pointed out, we create the hash table even when
> > lookup (i.g., create_it is false), which is bad. So I think we can
> > have pgstat_get_replslot_entry() return NULL without creating the hash
> > table if the hash table is NULL and create_it is false so that backend
> > processes don’t create the hash table, not via
> > backend_read_statsfile(). Or another idea would be to always create
> > the hash table in pgstat_read_statsfiles(). That way, it would
> > simplify the code but could waste the memory if there is no
> > replication slot.
> >
>
> If you create it after reading 'R' message as we do in the case of 'D'
> message then it won't waste any memory. So probably creating in
> pgstat_read_statsfiles() would be better unless you see some other
> problem with that.

Yeah, I think that's the approach I mentioned as the former. I’ll
change in the next version patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/