RE: Conflict detection for update_deleted in logical replication

On Friday, August 22, 2025 7:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> 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.

Removed these changes for now, will post again once the
main patches get pushed.

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

OK, I have moved these changes into the 0003 patch in the
latest version.

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

Attach the V65 patch set which addressed above and
Shveta's comments[1].

[1] https://www.postgresql.org/message-id/CAJpy0uBFB6K2ZoLebLCBfG%2B2edu63dU5oS1C6MqcnfcQj4CofQ%40mail.gmail.com

Best Regards,
Hou zj