On Thu, Dec 21, 2023 at 8:47 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.
>
It depends on how one uses the function. For example, if one finds
there is a conflict via pg_get_replication_slots() and wants to check
the reason for the same via this new function then it would give the
correct answer. Now, if we think it is okay to have two columns
'conflicting' and 'conflict_reason/conflict_cause' to be returned by
pg_get_replication_slots() then that should serve the purpose as well
but one can argue that 'conflicting' is deducible from
'conflict_reason'.
The other thing we want to decide is how useful will it be to display
this information via pg_replication_slots view considering we already
have a boolean for it. Shall we add yet another column in the view and
if so, it would probably be a string rather than an enum? Now, if we
do so, we still want function pg_get_replication_slots() or
pg_get_slot_invalidation_cause() to return an enum value as we want
that to be synced/copied to standby.
--
With Regards,
Amit Kapila.