On Wed, Apr 21, 2021 at 12:50 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Apr 20, 2021 at 7:54 PM Masahiko Sawada <sawada.m...@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/