Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit b29cbd3

Browse files
author
Amit Kapila
committed
Fix the handling of the failover option in subscription commands.
Do not allow ALTER SUBSCRIPTION ... SET (failover = on|off) in a transaction block as the changed failover option of the slot can't be rolled back. For the same reason, we refrain from altering the replication slot's failover property if the subscription is created with a valid slot_name and create_slot=false. Reprted-by: Kuroda Hayato Author: Hou Zhijie Reviewed-by: Shveta Malik, Bertrand Drouvot, Kuroda Hayato Discussion: https://postgr.es/m/OS0PR01MB57165542B09DFA4943830BF294082@OS0PR01MB5716.jpnprd01.prod.outlook.com
1 parent 480bc6e commit b29cbd3

File tree

8 files changed

+52
-67
lines changed

8 files changed

+52
-67
lines changed

doc/src/sgml/ref/alter_subscription.sgml

+4-3
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,11 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
6666
</para>
6767

6868
<para>
69-
Commands <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command> and
69+
Commands <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command>,
7070
<command>ALTER SUBSCRIPTION ... {SET|ADD|DROP} PUBLICATION ...</command>
71-
with <literal>refresh</literal> option as <literal>true</literal> cannot be
72-
executed inside a transaction block.
71+
with <literal>refresh</literal> option as <literal>true</literal> and
72+
<command>ALTER SUBSCRIPTION ... SET (failover = on|off)</command>
73+
cannot be executed inside a transaction block.
7374

7475
These commands also cannot be executed when the subscription has
7576
<link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>

doc/src/sgml/ref/create_subscription.sgml

+16-2
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,7 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
122122
(You cannot combine setting <literal>connect</literal>
123123
to <literal>false</literal> with
124124
setting <literal>create_slot</literal>, <literal>enabled</literal>,
125-
<literal>copy_data</literal>, or <literal>failover</literal> to
126-
<literal>true</literal>.)
125+
or <literal>copy_data</literal> to <literal>true</literal>.)
127126
</para>
128127

129128
<para>
@@ -183,6 +182,21 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
183182
<xref linkend="logical-replication-subscription-examples-deferred-slot"/>
184183
for examples.
185184
</para>
185+
186+
<para>
187+
When setting <literal>slot_name</literal> to a valid name and
188+
<literal>create_slot</literal> to false, the
189+
<literal>failover</literal> property value of the named slot may
190+
differ from the counterpart <literal>failover</literal> parameter
191+
specified in the subscription. Always ensure the slot property
192+
<literal>failover</literal> matches the counterpart parameter of the
193+
subscription and vice versa. Otherwise, the slot on the publisher may
194+
behave differently from what these subscription options say: for
195+
example, the slot on the publisher could either be synced to the
196+
standbys even when the subscription's <literal>failover</literal>
197+
option is disabled or could be disabled for sync even when the
198+
subscription's <literal>failover</literal> option is enabled.
199+
</para>
186200
</listitem>
187201
</varlistentry>
188202
</variablelist>

doc/src/sgml/ref/pg_dump.sgml

+1-5
Original file line numberDiff line numberDiff line change
@@ -1611,11 +1611,7 @@ CREATE DATABASE foo WITH TEMPLATE template0;
16111611
dump can be restored without requiring network access to the remote
16121612
servers. It is then up to the user to reactivate the subscriptions in a
16131613
suitable way. If the involved hosts have changed, the connection
1614-
information might have to be changed. If the subscription needs to
1615-
be enabled for
1616-
<link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>,
1617-
execute <link linkend="sql-altersubscription-params-set"><literal>ALTER SUBSCRIPTION ... SET (failover = true)</literal></link>
1618-
after the slot has been created. It might also be appropriate to
1614+
information might have to be changed. It might also be appropriate to
16191615
truncate the target tables before initiating a new full table copy. If users
16201616
intend to copy initial data during refresh they must create the slot with
16211617
<literal>two_phase = false</literal>. After the initial sync, the

src/backend/commands/subscriptioncmds.c

+6-22
Original file line numberDiff line numberDiff line change
@@ -401,13 +401,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
401401
errmsg("%s and %s are mutually exclusive options",
402402
"connect = false", "copy_data = true")));
403403

404-
if (opts->failover &&
405-
IsSet(opts->specified_opts, SUBOPT_FAILOVER))
406-
ereport(ERROR,
407-
(errcode(ERRCODE_SYNTAX_ERROR),
408-
errmsg("%s and %s are mutually exclusive options",
409-
"connect = false", "failover = true")));
410-
411404
/* Change the defaults of other options. */
412405
opts->enabled = false;
413406
opts->create_slot = false;
@@ -836,21 +829,6 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
836829
(errmsg("created replication slot \"%s\" on publisher",
837830
opts.slot_name)));
838831
}
839-
840-
/*
841-
* If the slot_name is specified without the create_slot option,
842-
* it is possible that the user intends to use an existing slot on
843-
* the publisher, so here we alter the failover property of the
844-
* slot to match the failover value in subscription.
845-
*
846-
* We do not need to change the failover to false if the server
847-
* does not support failover (e.g. pre-PG17).
848-
*/
849-
else if (opts.slot_name &&
850-
(opts.failover || walrcv_server_version(wrconn) >= 170000))
851-
{
852-
walrcv_alter_slot(wrconn, opts.slot_name, opts.failover);
853-
}
854832
}
855833
PG_FINALLY();
856834
{
@@ -1267,6 +1245,12 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
12671245
errmsg("cannot set %s for enabled subscription",
12681246
"failover")));
12691247

1248+
/*
1249+
* The changed failover option of the slot can't be rolled
1250+
* back.
1251+
*/
1252+
PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ... SET (failover)");
1253+
12701254
values[Anum_pg_subscription_subfailover - 1] =
12711255
BoolGetDatum(opts.failover);
12721256
replaces[Anum_pg_subscription_subfailover - 1] = true;

src/bin/pg_dump/pg_dump.c

+12-15
Original file line numberDiff line numberDiff line change
@@ -4804,12 +4804,17 @@ getSubscriptions(Archive *fout)
48044804

48054805
if (dopt->binary_upgrade && fout->remoteVersion >= 170000)
48064806
appendPQExpBufferStr(query, " o.remote_lsn AS suboriginremotelsn,\n"
4807-
" s.subenabled,\n"
4808-
" s.subfailover\n");
4807+
" s.subenabled,\n");
48094808
else
48104809
appendPQExpBufferStr(query, " NULL AS suboriginremotelsn,\n"
4811-
" false AS subenabled,\n"
4812-
" false AS subfailover\n");
4810+
" false AS subenabled,\n");
4811+
4812+
if (fout->remoteVersion >= 170000)
4813+
appendPQExpBufferStr(query,
4814+
" s.subfailover\n");
4815+
else
4816+
appendPQExpBuffer(query,
4817+
" false AS subfailover\n");
48134818

48144819
appendPQExpBufferStr(query,
48154820
"FROM pg_subscription s\n");
@@ -5132,6 +5137,9 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo)
51325137
if (strcmp(subinfo->subrunasowner, "t") == 0)
51335138
appendPQExpBufferStr(query, ", run_as_owner = true");
51345139

5140+
if (strcmp(subinfo->subfailover, "t") == 0)
5141+
appendPQExpBufferStr(query, ", failover = true");
5142+
51355143
if (strcmp(subinfo->subsynccommit, "off") != 0)
51365144
appendPQExpBuffer(query, ", synchronous_commit = %s", fmtId(subinfo->subsynccommit));
51375145

@@ -5165,17 +5173,6 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo)
51655173
appendPQExpBuffer(query, ", '%s');\n", subinfo->suboriginremotelsn);
51665174
}
51675175

5168-
if (strcmp(subinfo->subfailover, "t") == 0)
5169-
{
5170-
/*
5171-
* Enable the failover to allow the subscription's slot to be
5172-
* synced to the standbys after the upgrade.
5173-
*/
5174-
appendPQExpBufferStr(query,
5175-
"\n-- For binary upgrade, must preserve the subscriber's failover option.\n");
5176-
appendPQExpBuffer(query, "ALTER SUBSCRIPTION %s SET(failover = true);\n", qsubname);
5177-
}
5178-
51795176
if (strcmp(subinfo->subenabled, "t") == 0)
51805177
{
51815178
/*

src/test/recovery/t/040_standby_failover_slots_sync.pl

+3-17
Original file line numberDiff line numberDiff line change
@@ -42,26 +42,12 @@
4242
SELECT current_timestamp;
4343
]);
4444

45-
# Create a slot on the publisher with failover disabled
46-
$publisher->safe_psql('postgres',
47-
"SELECT 'init' FROM pg_create_logical_replication_slot('lsub1_slot', 'pgoutput', false, false, false);"
48-
);
49-
50-
# Confirm that the failover flag on the slot is turned off
51-
is( $publisher->safe_psql(
52-
'postgres',
53-
q{SELECT failover from pg_replication_slots WHERE slot_name = 'lsub1_slot';}
54-
),
55-
"f",
56-
'logical slot has failover false on the publisher');
57-
58-
# Create a subscription (using the same slot created above) that enables
59-
# failover.
45+
# Create a subscription that enables failover.
6046
$subscriber1->safe_psql('postgres',
61-
"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);"
47+
"CREATE SUBSCRIPTION regress_mysub1 CONNECTION '$publisher_connstr' PUBLICATION regress_mypub WITH (slot_name = lsub1_slot, copy_data = false, failover = true, enabled = false);"
6248
);
6349

64-
# Confirm that the failover flag on the slot has now been turned on
50+
# Confirm that the failover flag on the slot is turned on
6551
is( $publisher->safe_psql(
6652
'postgres',
6753
q{SELECT failover from pg_replication_slots WHERE slot_name = 'lsub1_slot';}

src/test/regress/expected/subscription.out

+5-2
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,6 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PU
8989
ERROR: connect = false and enabled = true are mutually exclusive options
9090
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true);
9191
ERROR: connect = false and create_slot = true are mutually exclusive options
92-
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, failover = true);
93-
ERROR: connect = false and failover = true are mutually exclusive options
9492
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
9593
ERROR: slot_name = NONE and enabled = true are mutually exclusive options
9694
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;
472470
SET SESSION AUTHORIZATION regress_subscription_user3;
473471
ALTER SUBSCRIPTION regress_testsub RENAME TO regress_testsub2;
474472
ERROR: permission denied for database regression
473+
-- fail - cannot do ALTER SUBSCRIPTION SET (failover) inside transaction block
474+
BEGIN;
475+
ALTER SUBSCRIPTION regress_testsub SET (failover);
476+
ERROR: ALTER SUBSCRIPTION ... SET (failover) cannot run inside a transaction block
477+
COMMIT;
475478
-- ok, owning it is enough for this stuff
476479
ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);
477480
DROP SUBSCRIPTION regress_testsub;

src/test/regress/sql/subscription.sql

+5-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ SET SESSION AUTHORIZATION 'regress_subscription_user';
5454
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, copy_data = true);
5555
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, enabled = true);
5656
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true);
57-
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, failover = true);
5857
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
5958
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false, create_slot = true);
6059
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;
333332
SET SESSION AUTHORIZATION regress_subscription_user3;
334333
ALTER SUBSCRIPTION regress_testsub RENAME TO regress_testsub2;
335334

335+
-- fail - cannot do ALTER SUBSCRIPTION SET (failover) inside transaction block
336+
BEGIN;
337+
ALTER SUBSCRIPTION regress_testsub SET (failover);
338+
COMMIT;
339+
336340
-- ok, owning it is enough for this stuff
337341
ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);
338342
DROP SUBSCRIPTION regress_testsub;

0 commit comments

Comments
 (0)