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