PS_NITPICKS_20240717_2PC_V180002.txt

text/plain

Filename: PS_NITPICKS_20240717_2PC_V180002.txt
Type: text/plain
Part: 0
Message: Re: Slow catchup of 2PC (twophase) transactions on replica in LR
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index d80d60c..5c6f0a5 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1262,7 +1262,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 				if (IsSet(opts.specified_opts, SUBOPT_FAILOVER))
 				{
 					/*
-					 * First mark the needs to alter the replication slot.
+					 * First, mark the need to alter the replication slot.
 					 * Failover option is controlled by both the publisher (as
 					 * a slot option) and the subscriber (as a subscription
 					 * option).
@@ -1280,7 +1280,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 				if (IsSet(opts.specified_opts, SUBOPT_TWOPHASE_COMMIT))
 				{
 					/*
-					 * First check the need to alter the replication slot.
+					 * First, check the need to alter the replication slot.
 					 * Two_phase option is controlled by both the publisher
 					 * (as a slot option) and the subscriber (as a
 					 * subscription option). The slot option must be altered
@@ -1302,11 +1302,9 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 										isTopLevel);
 
 					/*
-					 * If the wo_phase slot option must be altered, this
-					 * cannot be altered with slot_name simultaneously. The
-					 * latter part refers to the pre-set slot name and tries
-					 * to modify the slot option, so changing both does not
-					 * make sense.
+					 * Modifying the two_phase slot option requires a slot
+					 * lookup by slot name, so changing the slot name
+					 * at the same time is not allowed.
 					 */
 					if (update_two_phase &&
 						IsSet(opts.specified_opts, SUBOPT_SLOT_NAME))
diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl
index f56dff4..26b9e2c 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -423,9 +423,12 @@ $result = $node_subscriber->safe_psql('postgres',
 	"SELECT count(*) FROM pg_prepared_xacts;");
 is($result, qq(0), 'should be no prepared transactions on subscriber');
 
-# Toggle the two_phase to "true" *before* the COMMIT PREPARED. Since we are the
-# special path for the case where both two_phase and failover are altered, it
-# is also set to "true".
+###############################
+# Toggle the two_phase to "true" before the COMMIT PREPARED.
+#
+# Since we are the special path for the case where both two_phase
+# and failover are altered, it is also set to "true".
+###############################
 $node_subscriber->safe_psql('postgres',
     "ALTER SUBSCRIPTION tap_sub_copy DISABLE;");
 $node_subscriber->poll_query_until('postgres',