Re: Resetting spilled txn statistics in pg_stat_replication

On Mon, Oct 5, 2020 at 1:26 PM Masahiko Sawada
<masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
>
> On Sat, 3 Oct 2020 at 16:55, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Sat, Oct 3, 2020 at 9:26 AM Masahiko Sawada
> > <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> > >
> > > 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.
> > >
> >
> > I think that would happen anyway as we need to add more columns in
> > view for the physical slots.
>
> I think it depends; adding more columns to the view might not break
> tools if the query used in the tool explicitly specifies columns.
>

What if it uses Select * ...? It might not be advisable to assume how
the user might fetch data.

> OTOH
> if the view starts to show more rows, the tool will need to have the
> condition to get the same result as before.
>
> >
> > > 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.
> > >
> >
> > I am not sure at this stage but I think we will add the additional
> > stats for physical slots once we have any in this view itself. I would
> > like to avoid adding separate views if possible. The only reason to
> > omit physical slots at this stage is that we don't have any stats for
> > the same.
>
> I also prefer not to have separate views. I'm concerned about the
> compatibility I explained above but at the same time I agree that it
> doesn't make sense to show the stats always having nothing. Since
> given you and Dilip agreed on that, I also agree with that.
>

Okay.

> >
> > > 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.
> > >
> >
> > Hmm, I don't think it is shown intentionally in those views. I think
> > what is happening in other views is that it has been initialized with
> > some value when we read the stats and then while updating and or
> > writing because we don't change the stat_reset_timestamp, it displays
> > the same value as initialized at the time of read. Now, because in
> > pgstat_replslot_index() we always initialize the replSlotStats it
> > would overwrite any previous value we have set during read and display
> > the stat_reset as empty for replication slots. If we stop initializing
> > the replSlotStats in pgstat_replslot_index() then we will see similar
> > behavior as other views have. So even if we want to change then
> > probably we should stop initialization in pgstat_replslot_index but I
> > don't think that is necessarily better behavior because the
> > description of the parameter doesn't indicate any such thing.
>
> Understood. I agreed that the newly created slot doesn't have
> reset_timestamp. Looking at pg_stat_database, a view whose rows are
> added dynamically unlike other stat views, the newly created database
> doesn't have reset_timestamp. But given we clear the stats for a slot
> at pgstat_replslot_index(), why do we need to initialize the
> reset_timestamp with globalStats.stat_reset_timestamp when reading the
> stats file? Even if we could not find any slot stats in the stats file
> the view won’t show anything.
>

It was mainly for a code consistency point of view. Also, we will
clear the data in pgstat_replslot_index only for new slots, not for
existing slots. It might be used when we can't load the statsfile as
per comment in code ("Set the current timestamp (will be kept only in
case we can't load an existing statsfile)).

--
With Regards,
Amit Kapila.