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>, vignesh C <vignesh21@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-06-06T11:33:47Z
Lists: pgsql-hackers 
On Wed, Jun 4, 2025 at 4:12 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here is the V33 patch set which includes the following changes:
>

Few comments:
1.
+ if (sub->enabled)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot set option %s for enabled subscription",
+ "retain_conflict_info")));

Isn't it better to call CheckAlterSubOption() for this check, as we do
for failover and two_phase options?

2.
postgres=# Alter subscription sub1 set (retain_conflict_info=true);
ERROR:  cannot set option retain_conflict_info for enabled subscription
postgres=# Alter subscription sub1 disable;
ALTER SUBSCRIPTION
postgres=# Alter subscription sub1 set (retain_conflict_info=true);
WARNING:  information for detecting conflicts cannot be purged when
the subscription is disabled
ALTER SUBSCRIPTION

The above looks odd to me because first we didn't allow setting the
option for enabled subscription, and then when the user disabled the
subscription, a WARNING is issued. Isn't it better to give NOTICE
like: "enable the subscription to avoid accumulating deleted rows for
detecting conflicts" in the above case?

Also in this,
postgres=# Alter subscription sub1 set (retain_conflict_info=true);
WARNING:  information for detecting conflicts cannot be fully retained
when "track_commit_timestamp" is disabled
WARNING:  information for detecting conflicts cannot be purged when
the subscription is disabled
ALTER SUBSCRIPTION

What do we mean by this WARNING message? If track_commit_timestamp is
not enabled, we won't be able to detect certain conflicts, including
update_delete, but how can it lead to not retaining information
required for conflict detection? BTW, shall we consider giving ERROR
instead of WARNING for this case because without
track_commit_timestamp, there is no benefit in retaining deleted rows?
If we just want to make the user aware to enable
track_commit_timestamp to detect conflicts, then we can even consider
making this a NOTICE.

postgres=# Alter subscription sub1 Enable;
ALTER SUBSCRIPTION
postgres=# Alter subscription sub1 set (retain_conflict_info=false);
ERROR:  cannot set option retain_conflict_info for enabled subscription
postgres=# Alter subscription sub1 disable;
WARNING:  information for detecting conflicts cannot be purged when
the subscription is disabled
ALTER SUBSCRIPTION

Here, we should have a WARNING like: "deleted rows to detect conflicts
would not be removed till the subscription is enabled"; this should be
followed by errdetail like: "Consider setting retain_conflict_info to
false."

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