Re: Track in pg_replication_slots the reason why slots conflict?

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:
=========================
*
+       <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.