Re: Replication slot stats misgivings

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.

--
With Regards,
Amit Kapila.