Re: Function to get invalidation cause of a replication slot.

On Wed, Dec 20, 2023 at 12:46 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
>
> On 12/20/23 7:01 AM, shveta malik wrote:
> > Hello hackers,
> >
> > Attached is a patch which attempts to implement a new system function
> > pg_get_slot_invalidation_cause('slot_name')  to get invalidation cause
> > of a replication slot.
>
> Thanks! +1 for the idea to display this information through an SQL API.
>
> I wonder if we could add a new field to pg_replication_slots instead
> of creating a new function.
>

Yeah, that is another option. We already have a boolean column
'conflicting' in pg_replication_slots, so are you suggesting adding a
new column like 'conflict_cause'? I feel it is okay to have an SQL
function for this as it may be primarily used for internal purposes or
for some specific users who want to dig deeper for the invalidation
cause.

> > One of the use case scenarios for this function is another ongoing
> > thread "Synchronizing slots from primary to standby" at [1].  This
> > function is needed there to replicate invalidation-cause of a logical
> > replication slot from the primary server to the hot standby. But this
> > is an independent requirement in itself and thus makes sense to have
> > it implemented separately.
>
> Agree.
>
> My thoughts about it:
>
> 1 ===
>
> "Currently, users do not have a way to know the invalidation cause"
>
> I'm not sure about it, I think one could see the reasons in the log file.
>
> 2 ===
>
> "This function returns NULL if the replication slot is not found"
>
> Why not returning an error instead? (like pg_drop_replication_slot() does for example)
>

+1. Also, the check for NULL argument should be there.

> FWIW, we'd not need to cover this case if the description would be added to pg_replication_slots.
>
> 3 ===
>
> +          <para>
> +           <literal>3</literal> = wal_level insufficient for the slot
> +          </para>
>
> wal_level insufficient on the primary maybe?
>
> 4 ===
>
> + * Returns ReplicationSlotInvalidationCause enum value for valid slot_name;
>
> I think it would be more user friendly to return a description instead of an enum value.
>

But it would be tricky to use at a place where we want to know the
enum value as in the case of the sync slots patch where we want to
copy the cause. I think it would be better to display the description
if we want to display it in the view.

--
With Regards,
Amit Kapila.