On Mon, 5 Oct 2020 at 17:50, Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Mon, Oct 5, 2020 at 1:26 PM Masahiko Sawada
> <masahiko.saw...@2ndquadrant.com> wrote:
> >
> > On Sat, 3 Oct 2020 at 16:55, Amit Kapila <amit.kapil...@gmail.com> wrote:
> > >
> > > On Sat, Oct 3, 2020 at 9:26 AM Masahiko Sawada
> > > <masahiko.saw...@2ndquadrant.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)).
>
Understood.
Looking at pgstat_reset_replslot_counter() in the v8 patch, even if we
pass a physical slot name to pg_stat_reset_replication_slot() a
PgStat_MsgResetreplslotcounter is sent to the stats collector. I’m
okay with not raising an error but maybe we can have it not to send
the message in that case.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
