Re: Resetting spilled txn statistics in pg_stat_replication

On Wed, 30 Sep 2020 at 18:10, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Sep 30, 2020 at 1:12 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Fri, Sep 25, 2020 at 4:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Sep 24, 2020 at 5:44 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > I have done some more testing of this patch especially for the case
> > > where we spill before streaming the transaction and found everything
> > > is working as expected. Additionally, I have changed a few more
> > > comments and ran pgindent. I am still not very sure whether we want to
> > > display physical slots in this view as all the stats are for logical
> > > slots but anyway we can add stats w.r.t physical slots in the future.
> > > I am fine either way (don't show physical slots in this view or show
> > > them but keep stats as 0). Let me know if you have any thoughts on
> > > these points, other than that I am happy with the current state of the
> > > patch.
> >
> > IMHO, It will make more sense to only show the logical replication
> > slots in this view.
> >
>

Thank you for updating the patch.

> Okay, Sawada-San, others, do you have any opinion on this matter?
>

When we discussed this before, I was thinking that we could have other
statistics for physical slots in the same statistics view in the
future. Having the view show only logical slots also makes sense to me
but I’m concerned a bit that we could break backward compatibility
that monitoring tools etc will be affected when the view starts to
show physical slots too. If the view shows only logical slots, it also
might be worth considering to have separate views for logical slots
and physical slots and having pg_stat_logical_replication_slots by
this change.

Here is my review comment on the v7 patch.

+ /*
+ * Set the same reset timestamp for all replication slots too.
+ */
+ for (i = 0; i < max_replication_slots; i++)
+ replSlotStats[i].stat_reset_timestamp =
globalStats.stat_reset_timestamp;
+

You added back the above code but since we clear the timestamps on
creation of a new slot they are not shown:

+ /* Register new slot */
+ memset(&replSlotStats[nReplSlotStats], 0, sizeof(PgStat_ReplSlotStats));
+ memcpy(&replSlotStats[nReplSlotStats].slotname, name, NAMEDATALEN);

Looking at other statistics views such as pg_stat_slru,
pg_stat_bgwriter, and pg_stat_archiver, they have a valid
reset_timestamp value from the beginning. That's why I removed that
code and assigned the timestamp when registering a new slot.

---
+ if (OidIsValid(slot->data.database))
+ pgstat_report_replslot(NameStr(slot->data.name), 0, 0, 0);

I think we can use SlotIsLogical() for this purpose. The same is true
when dropping a slot.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services