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

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.

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

--
Best Regards,
Zhijie Hou