From 6c6e97b396056ab864ac20bc8112523c583ed450 Mon Sep 17 00:00:00 2001 From: Hou Zhijie Date: Tue, 16 Apr 2024 11:31:32 +0800 Subject: [PATCH v2] Fix the handling of failover option in subscription commands. Disallow ALTER SUBSCRIPTION in a transaction block when the replication slot is to be altered, since that cannot be rolled back. During CREATE SUBSCRIPTION, refrain from altering the replication slot's failover property if the subscription is created with a valid slot_name and create_slot=false. This can enable users to execute the command within a transaction block. --- doc/src/sgml/ref/alter_subscription.sgml | 7 +++-- doc/src/sgml/ref/create_subscription.sgml | 15 +++++++++ src/backend/commands/subscriptioncmds.c | 31 ++++++------------- src/bin/pg_dump/pg_dump.c | 14 ++------- .../t/040_standby_failover_slots_sync.pl | 20 ++---------- src/test/regress/expected/subscription.out | 7 +++-- src/test/regress/sql/subscription.sql | 6 +++- 7 files changed, 44 insertions(+), 56 deletions(-) diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml index 413ce68ce2..a78c1c3a47 100644 --- a/doc/src/sgml/ref/alter_subscription.sgml +++ b/doc/src/sgml/ref/alter_subscription.sgml @@ -66,10 +66,11 @@ ALTER SUBSCRIPTION name RENAME TO < - Commands ALTER SUBSCRIPTION ... REFRESH PUBLICATION and + Commands ALTER SUBSCRIPTION ... REFRESH PUBLICATION, ALTER SUBSCRIPTION ... {SET|ADD|DROP} PUBLICATION ... - with refresh option as true cannot be - executed inside a transaction block. + with refresh option as true and + ALTER SUBSCRIPTION ... SET (failover = on|off) + cannot be executed inside a transaction block. These commands also cannot be executed when the subscription has two_phase diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml index 15794731bb..bf99d9f7f7 100644 --- a/doc/src/sgml/ref/create_subscription.sgml +++ b/doc/src/sgml/ref/create_subscription.sgml @@ -183,6 +183,21 @@ CREATE SUBSCRIPTION subscription_name for examples. + + + When setting slot_name to a valid name and + create_slot to false, the + failover property value of the named slot may + differ from the counterpart failover parameter + specified in the subscription. When creating the slot, ensure the slot + property failover matches the counterpart parameter + of the subscription. Otherwise, the slot on the publisher may behave + differently from what these subscription options say: for example, the + slot on the publisher could either be synced to the standbys even when + the subscription's failover option is disabled or + could be disabled for sync even when the subscription's + failover option is enabled. + diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 5a47fa984d..fab8f91d44 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -401,13 +401,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options, errmsg("%s and %s are mutually exclusive options", "connect = false", "copy_data = true"))); - if (opts->failover && - IsSet(opts->specified_opts, SUBOPT_FAILOVER)) - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("%s and %s are mutually exclusive options", - "connect = false", "failover = true"))); - /* Change the defaults of other options. */ opts->enabled = false; opts->create_slot = false; @@ -836,21 +829,6 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt, (errmsg("created replication slot \"%s\" on publisher", opts.slot_name))); } - - /* - * If the slot_name is specified without the create_slot option, - * it is possible that the user intends to use an existing slot on - * the publisher, so here we alter the failover property of the - * slot to match the failover value in subscription. - * - * We do not need to change the failover to false if the server - * does not support failover (e.g. pre-PG17). - */ - else if (opts.slot_name && - (opts.failover || walrcv_server_version(wrconn) >= 170000)) - { - walrcv_alter_slot(wrconn, opts.slot_name, opts.failover); - } } PG_FINALLY(); { @@ -1267,6 +1245,15 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, errmsg("cannot set %s for enabled subscription", "failover"))); + /* + * Since altering a replication slot is not transactional, + * rolling back the transaction leaves the altered + * replication slot. So we cannot run ALTER SUBSCRIPTION + * inside a transaction block if altering a replication + * slot's failover option. + */ + PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ... SET (failover)"); + values[Anum_pg_subscription_subfailover - 1] = BoolGetDatum(opts.failover); replaces[Anum_pg_subscription_subfailover - 1] = true; diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index c52e961b30..e71842a357 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -5132,6 +5132,9 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo) if (strcmp(subinfo->subrunasowner, "t") == 0) appendPQExpBufferStr(query, ", run_as_owner = true"); + if (strcmp(subinfo->subfailover, "t") == 0) + appendPQExpBufferStr(query, ", failover = true"); + if (strcmp(subinfo->subsynccommit, "off") != 0) appendPQExpBuffer(query, ", synchronous_commit = %s", fmtId(subinfo->subsynccommit)); @@ -5165,17 +5168,6 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo) appendPQExpBuffer(query, ", '%s');\n", subinfo->suboriginremotelsn); } - if (strcmp(subinfo->subfailover, "t") == 0) - { - /* - * Enable the failover to allow the subscription's slot to be - * synced to the standbys after the upgrade. - */ - appendPQExpBufferStr(query, - "\n-- For binary upgrade, must preserve the subscriber's failover option.\n"); - appendPQExpBuffer(query, "ALTER SUBSCRIPTION %s SET(failover = true);\n", qsubname); - } - if (strcmp(subinfo->subenabled, "t") == 0) { /* diff --git a/src/test/recovery/t/040_standby_failover_slots_sync.pl b/src/test/recovery/t/040_standby_failover_slots_sync.pl index 76545e3c74..12acf874d7 100644 --- a/src/test/recovery/t/040_standby_failover_slots_sync.pl +++ b/src/test/recovery/t/040_standby_failover_slots_sync.pl @@ -42,26 +42,12 @@ my $slot_creation_time_on_primary = $publisher->safe_psql( SELECT current_timestamp; ]); -# Create a slot on the publisher with failover disabled -$publisher->safe_psql('postgres', - "SELECT 'init' FROM pg_create_logical_replication_slot('lsub1_slot', 'pgoutput', false, false, false);" -); - -# Confirm that the failover flag on the slot is turned off -is( $publisher->safe_psql( - 'postgres', - q{SELECT failover from pg_replication_slots WHERE slot_name = 'lsub1_slot';} - ), - "f", - 'logical slot has failover false on the publisher'); - -# Create a subscription (using the same slot created above) that enables -# failover. +# Create a subscription that enables failover. $subscriber1->safe_psql('postgres', - "CREATE SUBSCRIPTION regress_mysub1 CONNECTION '$publisher_connstr' PUBLICATION regress_mypub WITH (slot_name = lsub1_slot, copy_data=false, failover = true, create_slot = false, enabled = false);" + "CREATE SUBSCRIPTION regress_mysub1 CONNECTION '$publisher_connstr' PUBLICATION regress_mypub WITH (slot_name = lsub1_slot, copy_data = false, failover = true, enabled = false);" ); -# Confirm that the failover flag on the slot has now been turned on +# Confirm that the failover flag on the slot is turned on is( $publisher->safe_psql( 'postgres', q{SELECT failover from pg_replication_slots WHERE slot_name = 'lsub1_slot';} diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out index 1eee6b17b8..0f2a25cdc1 100644 --- a/src/test/regress/expected/subscription.out +++ b/src/test/regress/expected/subscription.out @@ -89,8 +89,6 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PU ERROR: connect = false and enabled = true are mutually exclusive options CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true); ERROR: connect = false and create_slot = true are mutually exclusive options -CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, failover = true); -ERROR: connect = false and failover = true are mutually exclusive options CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true); ERROR: slot_name = NONE and enabled = true are mutually exclusive options CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false, create_slot = true); @@ -472,6 +470,11 @@ REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3; SET SESSION AUTHORIZATION regress_subscription_user3; ALTER SUBSCRIPTION regress_testsub RENAME TO regress_testsub2; ERROR: permission denied for database regression +-- fail - cannot do ALTER SUBSCRIPTION SET (failover) inside transaction block +BEGIN; +ALTER SUBSCRIPTION regress_testsub SET (failover); +ERROR: ALTER SUBSCRIPTION ... SET (failover) cannot run inside a transaction block +COMMIT; -- ok, owning it is enough for this stuff ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE); DROP SUBSCRIPTION regress_testsub; diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql index 1b2a23ba7b..3e5ba4cb8c 100644 --- a/src/test/regress/sql/subscription.sql +++ b/src/test/regress/sql/subscription.sql @@ -54,7 +54,6 @@ SET SESSION AUTHORIZATION 'regress_subscription_user'; CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, copy_data = true); CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, enabled = true); CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true); -CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, failover = true); CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true); CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false, create_slot = true); CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE); @@ -333,6 +332,11 @@ REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3; SET SESSION AUTHORIZATION regress_subscription_user3; ALTER SUBSCRIPTION regress_testsub RENAME TO regress_testsub2; +-- fail - cannot do ALTER SUBSCRIPTION SET (failover) inside transaction block +BEGIN; +ALTER SUBSCRIPTION regress_testsub SET (failover); +COMMIT; + -- ok, owning it is enough for this stuff ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE); DROP SUBSCRIPTION regress_testsub; -- 2.30.0.windows.2