Hi, I started to write this as a reply to https://postgr.es/m/20210318015105.dcfa4ceybdjubf2i%40alap3.anarazel.de but I think it doesn't really fit under that header anymore.
On 2021-03-17 18:51:05 -0700, Andres Freund wrote: > It does make it easier for the shared memory stats patch, because if > there's a fixed number + location, the relevant stats reporting doesn't > need to go through a hashtable with the associated locking. I guess > that may have colored my perception that it's better to just have a > statically sized memory allocation for this. Noteworthy that SLRU stats > are done in a fixed size allocation as well... As part of reviewing the replication slot stats patch I looked at replication slot stats a fair bit, and I've a few misgivings. First, about the pgstat.c side of things: - If somehow slot stat drop messages got lost (remember pgstat communication is lossy!), we'll just stop maintaining stats for slots created later, because there'll eventually be no space for keeping stats for another slot. - If max_replication_slots was lowered between a restart, pgstat_read_statfile() will happily write beyond the end of replSlotStats. - pgstat_reset_replslot_counter() acquires ReplicationSlotControlLock. I think pgstat.c has absolutely no business doing things on that level. - We do a linear search through all replication slots whenever receiving stats for a slot. Even though there'd be a perfectly good index to just use all throughout - the slots index itself. It looks to me like slots stat reports can be fairly frequent in some workloads, so that doesn't seem great. - PgStat_ReplSlotStats etc use slotname[NAMEDATALEN]. Why not just NameData? - pgstat_report_replslot() already has a lot of stats parameters, it seems likely that we'll get more. Seems like we should just use a struct of stats updates. And then more generally about the feature: - If a slot was used to stream out a large amount of changes (say an initial data load), but then replication is interrupted before the transaction is committed/aborted, stream_bytes will not reflect the many gigabytes of data we may have sent. - I seems weird that we went to the trouble of inventing replication slot stats, but then limit them to logical slots, and even there don't record the obvious things like the total amount of data sent. I think the best way to address the more fundamental "pgstat related" complaints is to change how replication slot stats are "addressed". Instead of using the slots name, report stats using the index in ReplicationSlotCtl->replication_slots. That removes the risk of running out of "replication slot stat slots": If we loose a drop message, the index eventually will be reused and we likely can detect that the stats were for a different slot by comparing the slot name. It also makes it easy to handle the issue of max_replication_slots being lowered and there still being stats for a slot - we simply can skip restoring that slots data, because we know the relevant slot can't exist anymore. And we can make the initial pgstat_report_replslot() during slot creation use a I'm wondering if we should just remove the slot name entirely from the pgstat.c side of things, and have pg_stat_get_replication_slots() inquire about slots by index as well and get the list of slots to report stats for from slot.c infrastructure. Greetings, Andres Freund
