On Thu, Dec 28, 2023 at 10:16 AM shveta malik <shveta.ma...@gmail.com> wrote: > > 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: > > =========================
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
v2-0001-Track-conflicting_cause-in-pg_replication_slots.patch
Description: Binary data