Re: Avoid resetting subscription two_phase state to pending when already enabled

Hi,

On Mon, Nov 2, 2026 at 10:04 AM Peter Smith wrote:
> IMO, the nested checking of opts.twophase in the ternary makes the
> patch code unnecessarily hard to follow across combinations.
>
> My suggestion is to implement it using more verbose, but easier-to-read code:
>
> /* Change system catalog accordingly */
> if (sub->twophasestate == LOGICALREP_TWOPHASE_STATE_DISABLED)
> {
> if (opts.twophase)
> {
> /* enable two_phase */
> values[Anum_pg_subscription_subtwophasestate - 1] =
> CharGetDatum(LOGICALREP_TWOPHASE_STATE_PENDING);
> replaces[Anum_pg_subscription_subtwophasestate - 1] = true;
> }
> }
> else if (!opts.twophase)
> {
> /* disable two_phase */
> values[Anum_pg_subscription_subtwophasestate - 1] =
> CharGetDatum(LOGICALREP_TWOPHASE_STATE_DISABLED);
> replaces[Anum_pg_subscription_subtwophasestate - 1] = true;
> }
>
> ~

Thank for the suggestion, but I personally don't prefer the longer,
nested style, as I don't find it clearly easier to read. So I would keep
the current version for now and see what the committer thinks.

--
Best Regards,
Zhijie Hou