From 65b8560bb035a347823d5b45d63c66d825123141 Mon Sep 17 00:00:00 2001 From: Zhijie Hou Date: Wed, 4 Nov 2026 20:17:33 +0800 Subject: [PATCH v3] Avoid resetting subscription two_phase state to pending when already enabled. In AlterSubscription(), executing ALTER SUBSCRIPTION ... SET (two_phase = true) unconditionally set subtwophasestate to LOGICALREP_TWOPHASE_STATE_PENDING ('p'), even when two_phase was already in LOGICALREP_TWOPHASE_STATE_ENABLED ('e') 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 'p' broke checks that rely on subtwophasestate being 'e': 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. Fix this by not resetting subtwophasestate to PENDING when two_phase is already in ENABLED state. Changing slot_name together with SET (two_phase = true) on an already ENABLED subscription likewise leaves the state ENABLED, so the apply worker no longer enables two_phase on the replacement slot. This is intentional: resetting the state would reintroduce the problems described above, and the documentation already requires the replacement slot's two_phase property to match the subscription option. Author: Zhijie Hou Reviewed-by: shveta malik Reviewed-by: Hayato Kuroda Reviewed-by: Bertrand Drouvot Reviewed-by: vignesh C Reviewed-by: Peter Smith Reviewed-by: Amit Kapila Discussion: https://postgr.es/m/8e85abdf-738b-4578-ac42-4dc8eaa061dd@gmail.com Backpatch-through: 18 --- src/backend/commands/subscriptioncmds.c | 19 +++++++++++++------ src/test/subscription/t/021_twophase.pl | 16 +++++++++++++--- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 36bb20a1f6e..e4031efd7aa 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -1963,12 +1963,19 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, errmsg("cannot disable \"two_phase\" when prepared transactions exist"), errhint("Resolve these transactions and try again."))); - /* Change system catalog accordingly */ - values[Anum_pg_subscription_subtwophasestate - 1] = - CharGetDatum(opts.twophase ? - LOGICALREP_TWOPHASE_STATE_PENDING : - LOGICALREP_TWOPHASE_STATE_DISABLED); - replaces[Anum_pg_subscription_subtwophasestate - 1] = true; + /* + * Change system catalog accordingly, but do not reset the + * state to PENDING if two_phase is already enabled. + */ + if (!(opts.twophase && + sub->twophasestate == LOGICALREP_TWOPHASE_STATE_ENABLED)) + { + values[Anum_pg_subscription_subtwophasestate - 1] = + CharGetDatum(opts.twophase ? + LOGICALREP_TWOPHASE_STATE_PENDING : + LOGICALREP_TWOPHASE_STATE_DISABLED); + replaces[Anum_pg_subscription_subtwophasestate - 1] = true; + } } if (IsSet(opts.specified_opts, SUBOPT_FAILOVER)) diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl index f755e748e9c..ede7614be5c 100644 --- a/src/test/subscription/t/021_twophase.pl +++ b/src/test/subscription/t/021_twophase.pl @@ -431,8 +431,8 @@ is($result, qq(2), 'replicated data in subscriber table'); $node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub"); ############################### -# Alter the subscription to set two_phase to false. -# Verify that the altered subscription reflects the new two_phase option. +# Verify that setting two_phase to true when it is already enabled does not +# overwrite subtwophasestate to 'p' (pending). ############################### # Confirm that the two-phase slot option is enabled before altering @@ -441,12 +441,22 @@ $result = $node_publisher->safe_psql('postgres', ); is($result, qq(t), 'two-phase is enabled'); -# Alter subscription two_phase to false $node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION tap_sub_copy DISABLE;"); $node_subscriber->poll_query_until('postgres', "SELECT count(*) = 0 FROM pg_stat_activity WHERE backend_type = 'logical replication apply worker'" ); +$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'); + +############################### +# Alter the subscription to set two_phase to false. +# Verify that the altered subscription reflects the new two_phase option. +############################### $node_subscriber->safe_psql( 'postgres', " ALTER SUBSCRIPTION tap_sub_copy SET (two_phase = false); -- 2.54.0 (Apple Git-157)