Horton

Re: Conflict detection for update_deleted in logical replication

shveta malik <shveta.malik@gmail.com>

View thread View on postgr.es
From: shveta malik <shveta.malik@gmail.com>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, vignesh C <vignesh21@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, shveta malik <shveta.malik@gmail.com>
Date: 2025-08-28T10:07:00Z
Lists: pgsql-hackers 
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

Commits

  1. Fix intermittent BF failures in 035_conflicts.

  2. Resume conflict-relevant data retention automatically.

  3. Fix intermittent test failure introduced in 6456c6e2c4.

  4. Fix Coverity issue reported in commit a850be2fe.

  5. Add test to prevent premature removal of conflict-relevant data.

  6. Post-commit review fixes for 228c370868.

  7. Add max_retention_duration option to subscriptions.

  8. Detect and report update_deleted conflicts.

  9. Preserve conflict-relevant data during logical replication.

  10. Integrate FullTransactionIds deeper into two-phase code

  11. Improve checks for GUC recovery_target_timeline

  12. Prevent excessive delays before launching new logrep workers.

  13. Keep WAL segments by slot's last saved restart LSN

  14. Rework some code handling pg_subscription data in psql and pg_dump

  15. Use generateClonedIndexStmt to propagate CREATE INDEX to partitions.