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
