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

On Thu, Oct 29, 2026 at 11:27 AM Zhijie Hou <houzhijie22(at)gmail(dot)com> wrote:
>
>
> Hi,
>
> 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:

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))

This simply says: skip the write if the user asked for true and it's already 'e'

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.

thanks
Shveta