Dear Amit,
> + /*
> + * Do not allow changing the option if the subscription is enabled. This
> + * is because both failover and two_phase options of the slot on the
> + * publisher cannot be modified if the slot is currently acquired by the
> + * existing walsender.
> + */
> + if (sub->enabled)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("cannot set %s for enabled subscription",
> + option)));
>
> As per my understanding, the above comment is not true when we are
> changing 'two_phase' option from 'false' to 'true' because in that
> case, the existing walsender will only change it. So, ideally, we can
> allow toggling two_phase from 'false' to 'true' without the above
> restriction.
Hmm, yes. In "false" -> "true" case, the parameter of the slot is not changed by
the backend process. In this case, the subtwophasestate is changed to PENDING
once, then the walsender will change to ENABLED based on the worker requests.
> If this is correct then we don't even need to error for the case
> "cannot alter two_phase when logical replication worker is still
> running" when 'two_phase' option is changed from 'false' to 'true'.
Basically right, one note is that there is an Assert in maybe_reread_subscription(),
it should be also modified.
> Now, assuming the above observations are correct, we may still want to
> have the same behavior when toggling two_phase option but we can at
> least note down that in the comments so that if required the same can
> be changed when toggling 'two_phase' option from 'false' to 'true' in
> future.
>
> Thoughts?
+1 to add comments in CheckAlterSubOption(). How about the below draft?
```
@@ -1089,6 +1089,12 @@ CheckAlterSubOption(Subscription *sub, const char *option,
* is because both failover and two_phase options of the slot on the
* publisher cannot be modified if the slot is currently acquired by the
* existing walsender.
+ *
+ * XXX: when toggling two_phase from "false" to "true", the slot parameter
+ * is not modified by the backend process so that the lock conflict won't
+ * occur. The restarted walsender will do the alternation. Therefore, we
+ * can allow to switch without the restriction. This can be changed in
+ * the future based on the requirement.
```
Best regards,
Hayato Kuroda
FUJITSU LIMITED
