Horton

RE: Conflict detection for update_deleted in logical replication

Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>

View thread View on postgr.es
From: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.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-25T04:36:11Z
Lists: pgsql-hackers

Attachments

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

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.