On Wed, Dec 27, 2023 at 4:16 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Dec 27, 2023 at 3:08 PM shveta malik <shveta.ma...@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: > ========================= > * > + <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