v8-0004-Add-force_alter-option-for-ALTER-SUBSCIRPTION-.-S.patch

From 0ed818f49046ec06c5d2f56928fd1a672892c6b8 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Fri, 19 Apr 2024 11:03:19 +0000
Subject: [PATCH v8 4/4] Add force_alter option for ALTER SUBSCIRPTION ... SET
 command

Previously, all prepared transactions on the standby were rolled back when
toggling two_phase from "on" to "off". However, this operation may not be
expected by users. To ensure users understand what happens, we added the
"force_alter" parameter. When two_phase is toggling to "off", and there are
prepared transactions, they will be aborted only when "force_alter" is set to
true. Otherwise, an ERROR occurs.
---
 doc/src/sgml/ref/alter_subscription.sgml      |  9 ++--
 src/backend/commands/subscriptioncmds.c       | 28 ++++++++++++-
 src/test/regress/expected/subscription.out    |  3 ++
 src/test/regress/sql/subscription.sql         |  3 ++
 src/test/subscription/t/099_twophase_added.pl | 42 +++++++++++++++----
 5 files changed, 72 insertions(+), 13 deletions(-)

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index e81661aa04..1324d6390a 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -257,9 +257,12 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
 
      <para>
       The <literal>two_phase</literal> parameter can only be altered when the
-      subscription is disabled. When altering the parameter from <literal>on</literal>
-      to <literal>off</literal>, the backend process checks prepared
-      transactions done by the logical replication worker and aborts them.
+      subscription is disabled. Altering the parameter from <literal>on</literal>
+      to <literal>off</literal> will be failed when there are prepared
+      transactions done by the logical replication worker. If you want to alter
+      the parameter forcibly in this case, <literal>force_alter</literal>
+      option must be set to <literal>on</literal> at the same time. If
+      specified, the backend process aborts prepared transactions.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index cabbf9eab9..8b4ea403a0 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -73,6 +73,7 @@
 #define SUBOPT_FAILOVER				0x00002000
 #define SUBOPT_LSN					0x00004000
 #define SUBOPT_ORIGIN				0x00008000
+#define SUBOPT_FORCE_ALTER			0x00010000
 
 /* check if the 'val' has 'bits' set */
 #define IsSet(val, bits)  (((val) & (bits)) == (bits))
@@ -100,6 +101,7 @@ typedef struct SubOpts
 	bool		failover;
 	char	   *origin;
 	XLogRecPtr	lsn;
+	bool		force_alter;
 } SubOpts;
 
 static List *fetch_table_list(WalReceiverConn *wrconn, List *publications);
@@ -162,6 +164,8 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 		opts->failover = false;
 	if (IsSet(supported_opts, SUBOPT_ORIGIN))
 		opts->origin = pstrdup(LOGICALREP_ORIGIN_ANY);
+	if (IsSet(supported_opts, SUBOPT_FORCE_ALTER))
+		opts->force_alter = false;
 
 	/* Parse options */
 	foreach(lc, stmt_options)
@@ -367,6 +371,15 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 			opts->specified_opts |= SUBOPT_LSN;
 			opts->lsn = lsn;
 		}
+		else if (IsSet(supported_opts, SUBOPT_FORCE_ALTER) &&
+				 strcmp(defel->defname, "force_alter") == 0)
+		{
+			if (IsSet(opts->specified_opts, SUBOPT_FORCE_ALTER))
+				errorConflictingDefElem(defel, pstate);
+
+			opts->specified_opts |= SUBOPT_FORCE_ALTER;
+			opts->force_alter = defGetBoolean(defel);
+		}
 		else
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1150,7 +1163,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 								  SUBOPT_DISABLE_ON_ERR |
 								  SUBOPT_PASSWORD_REQUIRED |
 								  SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER |
-								  SUBOPT_ORIGIN);
+								  SUBOPT_ORIGIN | SUBOPT_FORCE_ALTER);
 
 				parse_subscription_options(pstate, stmt->options,
 										   supported_opts, &opts);
@@ -1200,6 +1213,19 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 						if (sub->twophasestate == LOGICALREP_TWOPHASE_STATE_ENABLED &&
 							(prepared_xacts = GetGidListBySubid(subid)) != NIL)
 						{
+							/*
+							 * Abort prepared transactions only if
+							 * 'force_alter' option is true. Otherwise raise
+							 * an ERROR.
+							 */
+							if (!opts.force_alter)
+								ereport(ERROR,
+										(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+										 errmsg("cannot alter %s when there are prepared transactions",
+												"two_phase = off"),
+										 errhint("Resolve these transactions or set %s at the same time, and then try again.",
+												 "force_alter = true")));
+
 							/* Abort all listed transactions */
 							foreach_ptr(char, gid, prepared_xacts)
 								FinishPreparedTransaction(gid, false);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 51fa4b9690..f607045b28 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -370,6 +370,9 @@ ERROR:  two_phase requires a Boolean value
 CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, two_phase = true);
 WARNING:  subscription was created, but is not connected
 HINT:  To initiate replication, you must manually create the replication slot, enable the subscription, and refresh the subscription.
+-- fail - force_alter cannot be set alone
+ALTER SUBSCRIPTION regress_testsub SET (force_alter = true);
+ERROR:  force_alter must be specified with two_phase
 \dRs+
                                                                                                                 List of subscriptions
       Name       |           Owner           | Enabled | Publication | Binary | Streaming | Two-phase commit | Disable on error | Origin | Password required | Run as owner? | Failover | Synchronous commit |          Conninfo           | Skip LSN 
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index a3886d79ca..80ab4dd9bc 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -255,6 +255,9 @@ CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUB
 -- now it works
 CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, two_phase = true);
 
+-- fail - force_alter cannot be set alone
+ALTER SUBSCRIPTION regress_testsub SET (force_alter = true);
+
 \dRs+
 -- We can alter streaming when two_phase enabled
 ALTER SUBSCRIPTION regress_testsub SET (streaming = true);
diff --git a/src/test/subscription/t/099_twophase_added.pl b/src/test/subscription/t/099_twophase_added.pl
index 59baa3eadc..c826881802 100644
--- a/src/test/subscription/t/099_twophase_added.pl
+++ b/src/test/subscription/t/099_twophase_added.pl
@@ -95,7 +95,8 @@ is($result, q(5),
 # Check the case that prepared transactions exist on the subscriber node
 #
 # If the two_phase is altering from "on" to "off" and there are prepared
-# transactions on the subscriber, they must be aborted. This test checks it.
+# transactions on the subscriber, we must error out or abort all prepared
+# transactions. Below part checks both cases.
 
 # Prepare a transaction to insert some tuples into the table
 $node_publisher->safe_psql(
@@ -112,15 +113,38 @@ $result = $node_subscriber->safe_psql('postgres',
     "SELECT count(*) FROM pg_prepared_xacts;");
 is($result, q(1), "transaction has been prepared on subscriber");
 
-# Toggle the two_phase to "off" before the COMMIT PREPARED
-$node_subscriber->safe_psql(
-    'postgres', "
-    ALTER SUBSCRIPTION regress_sub DISABLE;
-    ALTER SUBSCRIPTION regress_sub SET (two_phase = off);
-    ALTER SUBSCRIPTION regress_sub ENABLE;");
 
-# Verify the prepared transaction are aborted because two_phase is changed to
-# "off".
+# Disable the subscription to alter the two_phase option
+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub DISABLE;");
+
+# Try altering the two_phase option to "off." The command will fail since there
+# is a prepared transaction and the force option is not specified.
+my $stdout;
+my $stderr;
+
+($result, $stdout, $stderr) = $node_subscriber->psql(
+	'postgres', "ALTER SUBSCRIPTION regress_sub SET (two_phase = off);");
+ok($stderr =~ /cannot alter two_phase = off when there are prepared transactions/,
+	'ALTER SUBSCRIPTION failed');
+
+# Verify the prepared transaction still exists
+$result = $node_subscriber->safe_psql('postgres',
+    "SELECT count(*) FROM pg_prepared_xacts;");
+is($result, q(1), "prepared transaction still exits");
+
+$log_offset = -s $node_subscriber->logfile;
+
+# Alter the two_phase with the force_alter option. Apart from the above, the
+# command will abort the prepared transaction and succeed. 
+$node_subscriber->safe_psql('postgres',
+    "ALTER SUBSCRIPTION regress_sub SET (two_phase = off, force_alter = true);");
+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub ENABLE;");
+
+# Verify the started worker recognized two_phase was disabled
+$node_subscriber->wait_for_log(
+	'logical replication apply worker for subscription "regress_sub" two_phase is DISABLED', $log_offset);
+
+# Verify the prepared transaction are aborted
 $result = $node_subscriber->safe_psql('postgres',
     "SELECT count(*) FROM pg_prepared_xacts;");
 is($result, q(0), "prepared transaction done by worker is aborted");
-- 
2.43.0