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: Nisha Moond <nisha.moond412@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, shveta malik <shveta.malik@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, vignesh C <vignesh21@gmail.com>
Date: 2025-08-22T11:16:48Z
Lists: pgsql-hackers 
On Thu, Aug 21, 2025 at 2:01 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here is the V64 patch set addressing this concern.
>

Few minor comments:
1.
static void
 process_syncing_tables_for_sync(XLogRecPtr current_lsn)
 {
- SpinLockAcquire(&MyLogicalRepWorker->relmutex);
+ SpinLockAcquire(&MyLogicalRepWorker->mutex);

Why is this change part of this patch? Please extract it as a separate
patch unless this change is related to this patch.

2.
 pg_stat_get_subscription(PG_FUNCTION_ARGS)
 {
-#define PG_STAT_GET_SUBSCRIPTION_COLS 10
+#define PG_STAT_GET_SUBSCRIPTION_COLS 11
  Oid subid = PG_ARGISNULL(0) ? InvalidOid : PG_GETARG_OID(0);
  int i;
  ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
@@ -1595,6 +1614,22 @@ pg_stat_get_subscription(PG_FUNCTION_ARGS)
  elog(ERROR, "unknown worker type");
  }

+ /*
+ * Use the worker's oldest_nonremovable_xid instead of
+ * pg_subscription.subretentionactive to determine whether retention
+ * is active, as retention resumption might not be complete even when
+ * subretentionactive is set to true; this is because the launcher
+ * assigns the initial oldest_nonremovable_xid after the apply worker
+ * updates the catalog (see resume_conflict_info_retention).
+ *
+ * Only the leader apply worker manages conflict retention (see
+ * maybe_advance_nonremovable_xid() for details).
+ */
+ if (!isParallelApplyWorker(&worker) && !isTablesyncWorker(&worker))
+ values[10] = TransactionIdIsValid(worker.oldest_nonremovable_xid);
+ else

The theory given in the comment sounds good to me but I still suggest
it is better to extract it into a separate patch, so that we can
analyse/test it separately. Also, it will reduce the patch size as
well.

3.
  /* Ensure that we can enable retain_dead_tuples */
  if (opts.retaindeadtuples)
- CheckSubDeadTupleRetention(true, !opts.enabled, WARNING);
+ CheckSubDeadTupleRetention(true, !opts.enabled, WARNING, true);
+
+ /* Notify that max_conflict_retention_duration is ineffective */
+ else if (opts.maxconflretention)
+ notify_ineffective_max_conflict_retention(true);

Can't we combine these checks by passing both parameters to
CheckSubDeadTupleRetention() and let that function handle all
inappropriate value cases? BTW, even for other places, see if you can
reduce the length of the function name
notify_ineffective_max_conflict_retention.

-- 
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.