Re: Conflict detection for update_deleted in logical replication

On Wed, Jun 4, 2025 at 4:12 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)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.