Re: Conflict detection for update_deleted in logical replication

On Tue, Jun 10, 2025 at 11:55 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> Here is the V35 patch set which includes the following changes:
>

Thank You for the patches, few comments:

1)
compute_min_nonremovable_xid:

+ /*
+ * Stop advancing xmin if an invalid non-removable transaction ID is
+ * found, otherwise update xmin.
+ */
+ if (!TransactionIdIsValid(nonremovable_xid))
+ *can_advance_xmin = false;

IMO, we can not have invalid nonremovable_xid here. It may be a
possibility in a later patch where we do stop-conflict-detection for a
worker and resets nonremovable_xid to Invalid. But in patch003, should
this rather be an Assert as we want to ensure that worker's oldest is
set to slot's-xmin by this time.

2)
postgres=# create subscription sub1 connection 'dbname=postgres
host=localhost user=shveta port=5433' publication pub1 WITH
(retain_conflict_info = true);
NOTICE: deleted rows will continue to accumulate for detecting
conflicts until the subscription is enabled
NOTICE: created replication slot "sub1" on publisher
CREATE SUBSCRIPTION

postgres=# alter subscription sub2 enable;
NOTICE: deleted rows will continue to accumulate for detecting
conflicts until the subscription is enabled

We should not have this 'NOTICE: deleted rows' in above 2 commands.

3)
+
+ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);
+DROP SUBSCRIPTION regress_testsub;

Shall we enable regress_testsub as well before we drop it to catch
unexpected notices/warnings if any?

4)
postgres=# alter subscription sub2 disable;
WARNING: commit timestamp and origin data required for detecting
conflicts won't be retained
HINT: Consider setting "track_commit_timestamp" to true.

Do we need "track_commit_timestamp" related WARNING while disabling
rci sub as well? I feel it should be there while enabling it alone.

thanks
Shveta