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? -- With Regards, Amit Kapila.
