Re: Conflict detection for update_deleted in logical replication

On Thu, Aug 21, 2025 at 2:01 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)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.