Re: Track in pg_replication_slots the reason why slots conflict?

On Wed, Dec 27, 2023 at 4:16 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Dec 27, 2023 at 3:08 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > PFA the patch which attempts to implement this.
> >
> > This patch changes the existing 'conflicting' field to
> > 'conflicting_cause' in pg_replication_slots.
> >
>
> This name sounds a bit odd to me, would it be better to name it as
> conflict_cause?
>
> A few other minor comments:
> =========================
> *
> + <structfield>conflicting_cause</structfield> <type>text</type>
> + </para>
> + <para>
> + Cause if this logical slot conflicted with recovery (and so is now
> + invalidated). It is always NULL for physical slots, as well as for
> + those logical slots which are not invalidated. Possible values are:
>
> Would it better to use description as follows:" Cause of logical
> slot's conflict with recovery. It is always NULL for physical slots,
> as well as for logical slots which are not invalidated. The non-NULL
> values indicate that the slot is marked as invalidated. Possible
> values are:
> .."
>
> *
> $res = $node_standby->safe_psql(
> 'postgres', qq(
> - select bool_and(conflicting) from pg_replication_slots;));
> + select bool_and(conflicting) from
> + (select conflicting_cause is not NULL as conflicting from
> pg_replication_slots);));
>
> Won't the query "select conflicting_cause is not NULL as conflicting
> from pg_replication_slots" can return false even for physical slots
> and then impact the result of the main query whereas the original
> query would seem to be simply ignoring physical slots? If this
> observation is correct then you might want to add a 'slot_type'
> condition in the new query.
>
> * After introducing check_slots_conflicting_cause(), do we need to
> have check_slots_conflicting_status()? Aren't both checking the same
> thing?

I think it is needed for the case where we want to check that there is
no conflict.

# Verify slots are reported as non conflicting in pg_replication_slots
check_slots_conflicting_status(0);

For the cases where there is conflict, I think
check_slots_conflicting_cause() can replace
check_slots_conflicting_status().

thanks
Shveta