From 0ed818f49046ec06c5d2f56928fd1a672892c6b8 Mon Sep 17 00:00:00 2001 From: Hayato Kuroda 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 name RENAME TO < The two_phase parameter can only be altered when the - subscription is disabled. When altering the parameter from on - to off, the backend process checks prepared - transactions done by the logical replication worker and aborts them. + subscription is disabled. Altering the parameter from on + to off 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, force_alter + option must be set to on at the same time. If + specified, the backend process aborts prepared transactions. 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