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
