Horton

Re: Conflict detection for update_deleted in logical replication

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: shveta malik <shveta.malik@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>
Date: 2025-09-09T11:00:43Z
Lists: pgsql-hackers 
On Tue, Sep 9, 2025 at 11:47 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here is V71 patch set which addressed above comments and [1].
>

IIUC, this patch after stopping the retention, it immediately starts
retrying to resume by transitioning through various phases. This can
consume CPU and network resources, if the apply_worker takes a long
time to catch up.

Few other comments:
1.
+ /*
+ * Return if the launcher has not initialized oldest_nonremovable_xid.
+ *
+ * It might seem feasible to directly check the conflict detection
+ * slot.xmin instead of relying on the launcher to assign the worker's
+ * oldest_nonremovable_xid; however, that could lead to a race condition
+ * where slot.xmin is set to InvalidTransactionId immediately after the
+ * check. In such cases, oldest_nonremovable_xid would no longer be
+ * protected by a replication slot and could become unreliable if a
+ * wraparound occurs.
+ */
+ if (!TransactionIdIsValid(nonremovable_xid))
+ return;

I understand the reason why we get assigned the worker's
non_removable_xid from launcher but all this makes the code complex to
understand. Isn't there any other way to achieve it? One naïve and
inefficient way could be to just restart the worker probably after
updating its retentionactive flag. I am not suggesting to make this
change but just a brainstorming point.

2. The function should_stop_conflict_info_retention() is invoked from
wait_for_local_flush() and then it can lead further state
transitioning which doesn't appear neat and makes code difficult to
understand.

3.
+ /*
+ * If conflict info retention was previously stopped due to a timeout, and
+ * the time required to advance the non-removable transaction ID has now
+ * decreased to within acceptable limits, resume the rentention.
+ */
+ if (!MySubscription->retentionactive)
+ {
+ rdt_data->phase = RDT_RESUME_CONFLICT_INFO_RETENTION;
+ process_rdt_phase_transition(rdt_data, false);
+ return;
+ }

In this check, where do we check the time has come in acceptable
range? Can you update comments to make it clear?

-- 
With Regards,
Amit Kapila.

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.