RE: Conflict detection for update_deleted in logical replication

On Fri, Jun 6, 2025 at 7:34 PM Amit Kapila wrote:

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

Moved.

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

Yes, a NOTICE would be better.

I think we normally only describe the current situation of the operation in a
NOTICE message, and the suggested message sounds like a hint.
So I used the following message:

"deleted rows will continue to accumulate for detecting conflicts until the subscription is enabled"

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

I think it's an unexpected case that track_commit_timestamp is not enabled, so
NOTICE may not be appropriate. Giving ERROR is also OK, but since user can
change the track_commit_timestamp setting at anytime after creating/modifying a
subscription, we can't catch all cases, so we considered simply issuing a
warning directly and document this case.

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

Changed as suggested.

Here is the V35 patch set which includes the following changes:

0001:
No change.

0002:
* Added an errdetail for reserved slot name error per off-list discussion with Shveta.
* Moves the codes in launcher's foreach loop to a new function to improve the readability.

0003:
* Addressed all above comments sent by Amit.
* Adjusted some comments per off-list discussion with Amit.
* Check track_commit_timestamp when enabling the subscription. This is to avoid
 passing track_commit_timestamp as a parameter to the check function.

0004:
Rebased

0005:
Rebased

0006:
Rebased

0007:
Rebased

0008:
Rebased

Best Regards,
Hou zj