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. > 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.
v8-0001-Track-statistics-for-spilling-of-changes-from-Reo.patch
Description: Binary data
v8-0002-Test-stats.patch
Description: Binary data
