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

On Thu, Oct 29, 2026 at 12:34 PM Zhijie Hou <houzhijie22(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Thu, Oct 29, 2026 at 02:33 PM shveta malik wrote:
> > On Thu, Oct 29, 2026 at 11:27 AM Zhijie Hou <houzhijie22(at)gmail(dot)com> wrote:
> >>
> >> While reviewing another logical replication thread, I noticed an issue
> >> with altering a subscription's two_phase option.
> >>
> >> In AlterSubscription(), executing ALTER SUBSCRIPTION ... SET (two_phase
> >> = true) unconditionally set subtwophasestate to
> >> LOGICALREP_TWOPHASE_STATE_PENDING, even when two_phase was already in
> >> LOGICALREP_TWOPHASE_STATE_ENABLED state.
> >>
> >> Because two_phase remains enabled on the publisher's replication slot
> >> and prepared transactions may already exist on the subscriber,
> >> overwriting the state to PENDING broke checks that rely on
> >> subtwophasestate being ENABLED:
> >>
> >> 1. A subsequent ALTER SUBSCRIPTION ... SET (two_phase = false) skipped
> >> the LookupGXactBySubid() check and disabled two_phase while prepared
> >> transactions still existed on the subscriber. When those transactions
> >> were later committed on the publisher, the publisher re-sent all of
> >> their changes as regular transactions instead of COMMIT PREPARED,
> >> leading to self-deadlocks, duplicate data, or orphaned prepared
> >> transactions.
> >>
> >> 2. ALTER SUBSCRIPTION ... REFRESH PUBLICATION (and SET/ADD PUBLICATION)
> >> with copy_data = true was allowed even though the publisher's slot still
> >> had two_phase enabled, allowing prepared transactions on newly added
> >> tables to be streamed during initial table synchronization and applied
> >> as empty prepares.
> >>
> >> I think we can fix this by not resetting subtwophasestate to PENDING
> >> when two_phase is already in the ENABLED state, as shown in the attached
> >> patch.
> >>
> >> This issue has existed since PG 18, when altering the two_phase option
> >> was introduced, so it should be back-patched down to that branch.
> >
> > The fix looks good. A few trivial things:
>
> Thanks for reviewing.
>
> >
> > 1)
> > + if (!opts.twophase ||
> > + sub->twophasestate != LOGICALREP_TWOPHASE_STATE_ENABLED)
> >
> > I find this condition slightly difficult to follow. Would below be
> > more readable?
> >
> > if (!(opts.twophase &&
> > sub->twophasestate == LOGICALREP_TWOPHASE_STATE_ENABLED))
>
> Changed as suggested.
>
> >
> > This simply says: skip the write if the user asked for true and it's already 'e'
>
> I did not change the comment as I perfer the current version.

Sure. I did not suggest changing the code-comment, I was just
describing the simplicity of the change.

> >
> > 2)
> > +# Verify that setting two_phase to true when it is already enabled does not
> > +# overwrite subtwophasestate to 'p' (pending).
> > +$node_subscriber->safe_psql('postgres',
> > + "ALTER SUBSCRIPTION tap_sub_copy SET (two_phase = true);");
> > +$result = $node_subscriber->safe_psql('postgres',
> > + "SELECT subtwophasestate FROM pg_subscription WHERE subname = 'tap_sub_copy';"
> > +);
> > +is($result, qq(e), 'two-phase subscription option remains enabled');
> > +
> >
> > This new test is added between the two steps of a test that verifies
> > disabling two-phase. The comment in previous test '# Alter
> > subscription two_phase to false' now seems misplaced and thus should
> > be changed/shifted.
>
> Agreed. I adjusted the test order to make it clear.
>
> Here is the V2 patch.

thanks, v2 LGTM.