Re: Replication slot stats misgivings

On Tue, Apr 20, 2021 at 9:08 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Apr 19, 2021 at 4:48 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Apr 19, 2021 at 2:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Apr 19, 2021 at 9:00 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Fri, Apr 16, 2021 at 2:58 PM Amit Kapila <amit.kapila16@gmail.com> 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:
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;

2) Should we change PGSTAT_FILE_FORMAT_ID as the statistic file format
has changed for replication statistics?

3) We can include PgStat_StatReplSlotEntry in typedefs.lst and remove
PgStat_ReplSlotStats from typedefs.lst

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))

Regards,
Vignesh