Hi,
On Thu, Oct 29, 2026 at 02:33 PM shveta malik wrote:
On Thu, Oct 29, 2026 at 11:27 AM Zhijie Hou <[email protected]> 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
From 65b8560bb035a347823d5b45d63c66d825123141 Mon Sep 17 00:00:00 2001
From: Zhijie Hou <[email protected]>
Date: Thu, 29 Oct 2026 14:58:32 +0800
Subject: [PATCH v2] 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.
Author: Zhijie Hou <[email protected]>
Reviewed-by: shveta malik <[email protected]>
Discussion: https://postgr.es/m/[email protected]
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)
