public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zhijie Hou (Fujitsu) <[email protected]>
To: Hayato Kuroda (Fujitsu) <[email protected]>
To: Amit Kapila <[email protected]>
To: shveta malik <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Drouvot, Bertrand <[email protected]>
Subject: RE: Disallow changing slot's failover option in transaction block
Date: Mon, 22 Apr 2024 00:27:42 +0000
Message-ID: <OS0PR01MB57162F84A9FB311AEBB125C994122@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <OSBPR01MB2552A17F786761759ED149A9F50D2@OSBPR01MB2552.jpnprd01.prod.outlook.com>
References: <OS0PR01MB57165542B09DFA4943830BF294082@OS0PR01MB5716.jpnprd01.prod.outlook.com>
<OSBPR01MB255229D89C6D4A298F19E016F5082@OSBPR01MB2552.jpnprd01.prod.outlook.com>
<CAJpy0uCCadKHEOT5cRT+EKwcmGuY2xxktaRgiNcS_RnFUcGYXQ@mail.gmail.com>
<CAA4eK1J9jg1wKcgkmjq6tzbR-dLgxiT=563Pq2zYKGBNrtQN+A@mail.gmail.com>
<OS0PR01MB57161131ABED42F449B5B1FF940D2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
<OSBPR01MB2552A17F786761759ED149A9F50D2@OSBPR01MB2552.jpnprd01.prod.outlook.com>
On Friday, April 19, 2024 10:54 AM Kuroda, Hayato/黒田 隼人 <[email protected]> wrote:
> In your patch, the pg_dump.c was updated. IIUC the main reason was that
> pg_restore executes some queries as single transactions so that ALTER
> SUBSCRIPTION cannot be done, right?
Yes, and please see below for other reasons.
> Also, failover was synchronized only when we were in the upgrade mode, but
> your patch seems to remove the condition. Can you clarify the reason?
We used ALTER SUBSCRIPTION in upgrade mode because it was not allowed to use
connect=false and failover=true together when CREATE SUBSCRIPTION. But since we
don't have this restriction anymore(we don't alter slot when creating sub
anymore), we can directly specify failover in CREATE SUBSCRIPTION and do that
in non-upgrade mode as well.
Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s comments.
[1] https://www.postgresql.org/message-id/CAJpy0uD3YOeDg-tTCUi3EZ8vznRDfDqO_k6LepJpXUV1Z_%3DgkA%40mail.g...
[2] https://www.postgresql.org/message-id/ZiIxuaiINsuaWuDK%40ip-10-97-1-34.eu-west-3.compute.internal
Best Regards,
Hou zj
Attachments:
[application/octet-stream] v3-0001-Fix-the-handling-of-failover-option-in-subscripti.patch (14.6K, ../OS0PR01MB57162F84A9FB311AEBB125C994122@OS0PR01MB5716.jpnprd01.prod.outlook.com/2-v3-0001-Fix-the-handling-of-failover-option-in-subscripti.patch)
download | inline diff:
From 26a946598acdeb08adaf9ff237c6b4b3a3d61a26 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <[email protected]>
Date: Tue, 16 Apr 2024 11:31:32 +0800
Subject: [PATCH v3] 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 | 18 +++++++++--
doc/src/sgml/ref/pg_dump.sgml | 6 +---
src/backend/commands/subscriptioncmds.c | 31 ++++++-------------
src/bin/pg_dump/pg_dump.c | 27 +++++++---------
.../t/040_standby_failover_slots_sync.pl | 20 ++----------
src/test/regress/expected/subscription.out | 7 +++--
src/test/regress/sql/subscription.sql | 6 +++-
8 files changed, 55 insertions(+), 67 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 <replaceable class="parameter">name</replaceable> RENAME TO <
</para>
<para>
- Commands <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command> and
+ Commands <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command>,
<command>ALTER SUBSCRIPTION ... {SET|ADD|DROP} PUBLICATION ...</command>
- with <literal>refresh</literal> option as <literal>true</literal> cannot be
- executed inside a transaction block.
+ with <literal>refresh</literal> option as <literal>true</literal> and
+ <command>ALTER SUBSCRIPTION ... SET (failover = on|off)</command>
+ cannot be executed inside a transaction block.
These commands also cannot be executed when the subscription has
<link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 15794731bb..740b7d9421 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -122,8 +122,7 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
(You cannot combine setting <literal>connect</literal>
to <literal>false</literal> with
setting <literal>create_slot</literal>, <literal>enabled</literal>,
- <literal>copy_data</literal>, or <literal>failover</literal> to
- <literal>true</literal>.)
+ or <literal>copy_data</literal> to <literal>true</literal>.)
</para>
<para>
@@ -183,6 +182,21 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
<xref linkend="logical-replication-subscription-examples-deferred-slot"/>
for examples.
</para>
+
+ <para>
+ When setting <literal>slot_name</literal> to a valid name and
+ <literal>create_slot</literal> to false, the
+ <literal>failover</literal> property value of the named slot may
+ differ from the counterpart <literal>failover</literal> parameter
+ specified in the subscription. Always ensure the slot property
+ <literal>failover</literal> matches the counterpart parameter of the
+ subscription and vice versa. 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 <literal>failover</literal>
+ option is disabled or could be disabled for sync even when the
+ subscription's <literal>failover</literal> option is enabled.
+ </para>
</listitem>
</varlistentry>
</variablelist>
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index b99793e414..671df4b60e 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -1611,11 +1611,7 @@ CREATE DATABASE foo WITH TEMPLATE template0;
dump can be restored without requiring network access to the remote
servers. It is then up to the user to reactivate the subscriptions in a
suitable way. If the involved hosts have changed, the connection
- information might have to be changed. If the subscription needs to
- be enabled for
- <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>,
- execute <link linkend="sql-altersubscription-params-set"><literal>ALTER SUBSCRIPTION ... SET (failover = true)</literal></link>
- after the slot has been created. It might also be appropriate to
+ information might have to be changed. It might also be appropriate to
truncate the target tables before initiating a new full table copy. If users
intend to copy initial data during refresh they must create the slot with
<literal>two_phase = false</literal>. After the initial sync, the
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 6d2f3fdef3..ae958d61fd 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4804,12 +4804,17 @@ getSubscriptions(Archive *fout)
if (dopt->binary_upgrade && fout->remoteVersion >= 170000)
appendPQExpBufferStr(query, " o.remote_lsn AS suboriginremotelsn,\n"
- " s.subenabled,\n"
- " s.subfailover\n");
+ " s.subenabled,\n");
else
appendPQExpBufferStr(query, " NULL AS suboriginremotelsn,\n"
- " false AS subenabled,\n"
- " false AS subfailover\n");
+ " false AS subenabled,\n");
+
+ if (fout->remoteVersion >= 170000)
+ appendPQExpBufferStr(query,
+ " s.subfailover\n");
+ else
+ appendPQExpBuffer(query,
+ " false AS subfailover\n");
appendPQExpBufferStr(query,
"FROM pg_subscription s\n");
@@ -5132,6 +5137,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 +5173,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
view thread (9+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: RE: Disallow changing slot's failover option in transaction block
In-Reply-To: <OS0PR01MB57162F84A9FB311AEBB125C994122@OS0PR01MB5716.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox