On Wed, Dec 20, 2023 at 12:43:45PM +0100, Drouvot, Bertrand wrote: > + <literal>3</literal> = wal_level insufficient on the primary > server > > "." is missing at the end (to be consistent with 1 and 2). Same > in the commit message. > > + * Returns ReplicationSlotInvalidationCause enum value for valid slot_name; > > Not sure the sentence should finish with ";". > > Another Nit is to add a comment in ReplicationSlotInvalidationCause > definition (slot.h) > that any new enum values (if any) should be added after the ones that are > already defined (to > provide some consistency across changes in this area if any). > > Except the above Nit(s) the patch LGTM.
This patch has a problem: this design can easily cause the report of data inconsistent with pg_replication_slots because the data returned by pg_get_replication_slots() and pg_get_slot_invalidation_cause() would happen across *two* function call contexts, no? So, it seems to me that this had better be integrated as an extra column of pg_get_replication_slots() to ensure the consistency of the data reported. And it's critical to get consistent data for monitoring. Not to mention that the lookup had better happen also while holding ReplicationSlotControlLock as well, which is also something InvalidatePossiblyObsoleteSlot() relies on. v2 ignores that. -- Michael
signature.asc
Description: PGP signature
