Re: Slow catchup of 2PC (twophase) transactions on replica in LR

On Thu, Jul 18, 2024 at 5:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> I'll continue my review and testing of the patch but I thought of
> sharing what I have done till now.
>

+ /*
+ * 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.

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

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?

--
With Regards,
Amit Kapila.