On Wed, Dec 20, 2023 at 2:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.
If arg is NULL, it will not come to this function call. It comes to
this function if the signature matches. That is why other functions
like pg_drop_replication_slot(), pg_replication_slot_advance() etc do
not have NULL arg check as well.
>
> > 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.
PFA v2 patch. Addressed below comments:
1) Added test in 019_replslot_limit.pl
2) 'pg_get_slot_invalidation_cause' now returns error if the given
slot does not exist
3) Corrected doc and commit msg.
thanks
Shveta