Re: Resetting spilled txn statistics in pg_stat_replication

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.

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

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

> ---
> + 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.
>

makes sense, so changed accordingly in the attached patch.

--
With Regards,
Amit Kapila.