PS_NITPICKS_20240718_2PC_V190002.txt

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f770e9c..a826be9 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1079,17 +1079,6 @@ CheckAlterSubOption(Subscription *sub, const char *option,
 		   strcmp(option, "two_phase") == 0);
 
 	/*
-	 * If the slot needs to be updated, the backend must connect to the
-	 * publisher and request the alteration. slot_name must be required at that
-	 * time.
-	 */
-	if (slot_needs_update && !sub->slotname)
-		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg("cannot set %s for a subscription that does not have a slot name",
-						option)));
-
-	/*
 	 * Do not allow changing the option if the subscription is enabled. This
 	 * is because both failover and two_phase options of the slot on the
 	 * publisher cannot be modified if the slot is currently acquired by the
@@ -1101,17 +1090,27 @@ CheckAlterSubOption(Subscription *sub, const char *option,
 				 errmsg("cannot set %s for enabled subscription",
 						option)));
 
-	/*
-	 * The changed option of the slot can't be rolled back, so disallow if we
-	 * are in a transaction block.
-	 */
 	if (slot_needs_update)
 	{
 		StringInfoData cmd;
 
+		/*
+		 * If the slot needs to be updated, the backend must connect to the
+		 * publisher and request the alteration. A slot_name is required at
+		 * that time.
+		 */
+		if (!sub->slotname)
+			ereport(ERROR,
+					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+					 errmsg("cannot set %s for a subscription that does not have a slot name",
+							option)));
+
+		/*
+		 * The changed option of the slot can't be rolled back, so disallow if we
+		 * are in a transaction block.
+		 */
 		initStringInfo(&cmd);
 		appendStringInfo(&cmd, "ALTER SUBSCRIPTION ... SET (%s)", option);
-
 		PreventInTransactionBlock(isTopLevel, cmd.data);
 		pfree(cmd.data);
 	}
@@ -1132,12 +1131,12 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	HeapTuple	tup;
 	Oid			subid;
 	bool		update_tuple = false;
+	bool		update_failover = false;
+	bool		update_two_phase = false;
 	Subscription *sub;
 	Form_pg_subscription form;
 	bits32		supported_opts;
 	SubOpts		opts = {0};
-	bool		update_failover = false;
-	bool		update_two_phase = false;
 
 	rel = table_open(SubscriptionRelationId, RowExclusiveLock);
 
@@ -1271,7 +1270,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).
@@ -1295,15 +1294,14 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 					 * subscription option). The slot option must be altered
 					 * only when changing "true" to "false".
 					 *
-					 * There is no need to do something remarkable regarding
+					 * There is no need to do anything remarkable for
 					 * the "false" to "true" case; the backend process alters
 					 * subtwophase to LOGICALREP_TWOPHASE_STATE_PENDING once.
 					 * After the subscription is enabled, a new logical
 					 * replication worker requests to change the two_phase
 					 * option of its slot from pending to true when the
-					 * initial data synchronization is done. The code path is
-					 * the same as the case in which two_phase is initially
-					 * set to true.
+					 * initial data synchronization is done. That code path is
+					 * the same as when two_phase is initially set to true.
 					 */
 					update_two_phase = !opts.twophase;
 
diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl
index c8fb3b7..91e2e17 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -428,9 +428,8 @@ 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".
+# Also, set failover to "true" to test the code path where
+# both two_phase and failover are altered at the same time.
 ###############################
 $node_subscriber->safe_psql('postgres',
     "ALTER SUBSCRIPTION tap_sub_copy DISABLE;");