Re: Track in pg_replication_slots the reason why slots conflict?

On Thu, Dec 28, 2023 at 10:16 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Dec 27, 2023 at 4:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Dec 27, 2023 at 3:08 PM shveta malik <shveta.malik@gmail.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:
> > =========================

Thanks for the feedback Amit.

> > *
> > +       <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().

I have removed check_slots_conflicting_status() and where it was
needed to check non-conflicting, I have added a simple query.

PFA the v2-patch with all your comments addressed.

thanks
Shveta