Re: Conflict detection for update_deleted in logical replication

On Thu, Aug 28, 2025 at 8:02 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
>
> I noticed that Cfbot failed to compile the document due to a typo after renaming
> the subscription option. Here are the updated V67 patches to fix that, only the doc
> in 0001 is modified.
>

Please find a  few comments:

patch001 :
1)
Assert is hit while testing patch001 alone:
TRAP: failed Assert("TransactionIdIsValid(nonremovable_xid)"), File:
"launcher.c", Line: 1394, PID: 55350.

Scenario:
I have 2 subs, both have stopped retention. Now on one of the sub, if I do this:

--switch off rdt:
alter subscription sub1 disable;
alter subscription sub1 set (retain_dead_tuples=off);
alter subscription sub1 enable;

--switch back rdt, launcher asserts after this
alter subscription sub1 disable;
alter subscription sub1 set (retain_dead_tuples=on);
alter subscription sub1 enable;

2)
+          Maximum duration for which this subscription's apply worker
is allowed
+          to retain the information useful for conflict detection when
+          <literal>retain_dead_tuples</literal> is enabled for the associated
+          subscriptions.

associated subscriptions --> associated subscription (since we are
talking about 'this subscription's apply worker')

3)
+          The information useful for conflict detection is no longer
retained if
+          all apply workers associated with the subscriptions, where
+          <literal>retain_dead_tuples</literal> is enabled, confirm that the
+          retention duration exceeded the

A trivial thing: 'retention duration has exceeded' sounds better to me.

~~

patch002:
(feel free to defer these comments if we are focusing on patch001 right now):

4)
stop_conflict_info_retention() has the correct message and detail in
patch01 while in patch02, it is switched back to the older one (wrong
one). Perhaps some merge mistake

5)
resume_conflict_info_retention() still refers to the old GUC name:
max_conflict_retention_duration.

6)
In compute_min_nonremovable_xid(), shall we have
Assert(TransactionIdIsValid(nonremovable_xid)) before assigning it to
worker->oldest_nonremovable_xid? Here:

+ nonremovable_xid = MyReplicationSlot->data.xmin;
+
+ SpinLockAcquire(&worker->relmutex);
+ worker->oldest_nonremovable_xid = nonremovable_xid;
+ SpinLockRelease(&worker->relmutex);

7)
Now since compute_min_nonremovable_xid() is also taking care of
assigning valid xid to the worker, shall we update that in comment as
well? We can have one more line:
'Additionally if any of the apply workers has invalid xid, assign
slot's xmin to it.'

thanks
Shveta