Re: Replication slot stats misgivings

On Wed, Apr 21, 2021 at 12:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Apr 20, 2021 at 7:54 PM Masahiko Sawada <sawada.mshk@gmail.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. I slightly prefer the former but what do you think?

Regards,

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