Re: Conflict detection for update_deleted in logical replication

On Wed, Jun 4, 2025 at 4:12 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> Here is the V33 patch set which includes the following changes:
>

Thank You for the patches, please find few comments for patch003:

1)
+ /*
+ * Skip the track_commit_timestamp check by passing it as
+ * true, since it has already been validated during CREATE
+ * SUBSCRIPTION and ALTER SUBSCRIPTION SET commands.
+ */
+ CheckSubConflictInfoRetention(sub->retainconflictinfo,
+ true, opts.enabled);
+

Is there a special reason for disabling WARNING while enabling the
subscription? If rci subscription was created in disabled state and
track_commit_timestamp was enabled at that time, then there will be no
WARNING. But while enabling the sub at a later stage, it may be
possible that track_commit_timestamp is off but rci as ON.

2)

* The worker has not yet started, so there is no valid
* non-removable transaction ID available for advancement.
*/
+ if (sub->retainconflictinfo)
+ can_advance_xmin = false;

Shall we change comment to:
Only advance xmin when all workers for rci enabled subscriptions are
up and running.

3)

Assert(MyReplicationSlot);
- Assert(TransactionIdIsValid(new_xmin));
Assert(TransactionIdPrecedesOrEquals(MyReplicationSlot->data.xmin,
new_xmin));

+ if (!TransactionIdIsValid(new_xmin))
+ return;

a)
Why have we replaced Assert with 'if' check? In which scenario do we
expect new_xmin as Invalid here?

b)
Even if we have if-check, shall it come before :
Assert(TransactionIdPrecedesOrEquals(MyReplicationSlot->data.xmin,
new_xmin));

4)
DisableSubscriptionAndExit:
+ /*
+ * Skip the track_commit_timestamp check by passing it as true, since it
+ * has already been validated during CREATE SUBSCRIPTION and ALTER
+ * SUBSCRIPTION SET commands.
+ */
+ CheckSubConflictInfoRetention(MySubscription->retainconflictinfo, true,
+ false);

This comment makes sense during alter-sub enable, here shall we change it to:
Skip the track_commit_timestamp check by passing it as true as it is
not needed to be checked during subscription-disable.

5)
postgres=# alter subscription sub3 set (retain_conflict_info=false);
ERROR: cannot set option retain_conflict_info for enabled subscription

Do we need this restriction during disable of rci as well?

6)
+ <para>
+ If the <link
linkend="sql-createsubscription-params-with-retain-conflict-info"><literal>retain_conflict_info</literal></link>
+ option is altered to <literal>false</literal> and no other subscription
+ has this option enabled, the additional replication slot that was created
+ to retain conflict information will be dropped.
+ </para>

It will be good to mention the slot name as well.

7)
+ * Verify that the max_active_replication_origins and max_replication_slots
+ * configurations specified are enough for creating the subscriptions. This is
+ * required to create the replication origin and the conflict detection slot
+ * for each subscription.
*/

We shall rephrase the comment, it gives the feeling that a 'conflict
detection slot' is needed for each subscription.

thanks
Shveta