On Tue, Apr 20, 2021 at 7:22 PM vignesh C <[email protected]> wrote: > > On Tue, Apr 20, 2021 at 9:08 AM Masahiko Sawada <[email protected]> wrote: > > > > On Mon, Apr 19, 2021 at 4:48 PM Masahiko Sawada <[email protected]> > > wrote: > > > > > > On Mon, Apr 19, 2021 at 2:14 PM Amit Kapila <[email protected]> > > > wrote: > > > > > > > > On Mon, Apr 19, 2021 at 9:00 AM Masahiko Sawada <[email protected]> > > > > wrote: > > > > > > > > > > On Fri, Apr 16, 2021 at 2:58 PM Amit Kapila <[email protected]> > > > > > wrote: > > > > > > > > > > > > > > > > > > 4. > > > > > > +CREATE VIEW pg_stat_replication_slots AS > > > > > > + SELECT > > > > > > + s.slot_name, > > > > > > + s.spill_txns, > > > > > > + s.spill_count, > > > > > > + s.spill_bytes, > > > > > > + s.stream_txns, > > > > > > + s.stream_count, > > > > > > + s.stream_bytes, > > > > > > + s.total_txns, > > > > > > + s.total_bytes, > > > > > > + s.stats_reset > > > > > > + FROM pg_replication_slots as r, > > > > > > + LATERAL pg_stat_get_replication_slot(slot_name) as s > > > > > > + WHERE r.datoid IS NOT NULL; -- excluding physical slots > > > > > > .. > > > > > > .. > > > > > > > > > > > > -/* Get the statistics for the replication slots */ > > > > > > +/* Get the statistics for the replication slot */ > > > > > > Datum > > > > > > -pg_stat_get_replication_slots(PG_FUNCTION_ARGS) > > > > > > +pg_stat_get_replication_slot(PG_FUNCTION_ARGS) > > > > > > { > > > > > > #define PG_STAT_GET_REPLICATION_SLOT_COLS 10 > > > > > > - ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; > > > > > > + text *slotname_text = PG_GETARG_TEXT_P(0); > > > > > > + NameData slotname; > > > > > > > > > > > > I think with the above changes getting all the slot stats has become > > > > > > much costlier. Is there any reason why can't we get all the stats > > > > > > from > > > > > > the new hash_table in one shot and return them to the user? > > > > > > > > > > I think the advantage of this approach would be that it can avoid > > > > > showing the stats for already-dropped slots. Like other statistics > > > > > views such as pg_stat_all_tables and pg_stat_all_functions, searching > > > > > the stats by the name got from pg_replication_slots can show only > > > > > available slot stats even if the hash table has garbage slot stats. > > > > > > > > > > > > > Sounds reasonable. However, if the create_slot message is missed, it > > > > will show an empty row for it. See below: > > > > > > > > postgres=# select slot_name, total_txns from pg_stat_replication_slots; > > > > slot_name | total_txns > > > > -----------+------------ > > > > s1 | 0 > > > > s2 | 0 > > > > | > > > > (3 rows) > > > > > > > > Here, I have manually via debugger skipped sending the create_slot > > > > message for the third slot and we are showing an empty for it. This > > > > won't happen for pg_stat_all_tables, as it will set 0 or other initial > > > > values in such a case. I think we need to address this case. > > > > > > Good catch. I think it's better to set 0 to all counters and NULL to > > > reset_stats. > > > > > > > > > > > > Given that pg_stat_replication_slots doesn’t show garbage slot stats > > > > > even if it has, I thought we can avoid checking those garbage stats > > > > > frequently. It should not essentially be a problem for the hash table > > > > > to have entries up to max_replication_slots regardless of live or > > > > > already-dropped. > > > > > > > > > > > > > Yeah, but I guess we still might not save much by not doing it, > > > > especially because for the other cases like tables/functions, we are > > > > doing it without any threshold limit. > > > > > > Agreed. > > > > > > I've attached the updated patch, please review it. > > > > I've attached the new version patch that fixed the compilation error > > reported off-line by Amit. > > Thanks for the updated patch, few comments:
Thank you for the review comments.
> 1) We can change "slotent = pgstat_get_replslot_entry(slotname,
> false);" to "return pgstat_get_replslot_entry(slotname, false);" and
> remove the slotent variable.
>
> + PgStat_StatReplSlotEntry *slotent = NULL;
> +
> backend_read_statsfile();
>
> - *nslots_p = nReplSlotStats;
> - return replSlotStats;
> + slotent = pgstat_get_replslot_entry(slotname, false);
> +
> + return slotent;
Fixed.
>
> 2) Should we change PGSTAT_FILE_FORMAT_ID as the statistic file format
> has changed for replication statistics?
The struct name is changed but I think the statistics file format has
not changed by this patch. No?
>
> 3) We can include PgStat_StatReplSlotEntry in typedefs.lst and remove
> PgStat_ReplSlotStats from typedefs.lst
Fixed.
>
> 4) Few indentation issues are there, we can run pgindent on pgstat.c changes:
> case 'R':
> - if
> (fread(&replSlotStats[nReplSlotStats], 1,
> sizeof(PgStat_ReplSlotStats), fpin)
> - != sizeof(PgStat_ReplSlotStats))
> + {
> + PgStat_StatReplSlotEntry slotbuf;
> + PgStat_StatReplSlotEntry *slotent;
> +
> + if (fread(&slotbuf, 1,
> sizeof(PgStat_StatReplSlotEntry), fpin)
> + != sizeof(PgStat_StatReplSlotEntry))
Fixed.
I've attached the patch. In addition to the test Vignesh prepared, I
added one test for the message for creating a slot that checks if the
statistics are initialized after re-creating the same name slot.
Please review it.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
v11-0001-Use-HTAB-for-replication-slot-statistics.patch
Description: Binary data
