Disallow changing slot's failover option in transaction block
Hi,
Kuroda-San reported an issue off-list that:
If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
and rollback, only the subscription option change can be rolled back, while the
replication slot's failover change is preserved.
This is because ALTER SUBSCRIPTION SET (failover) command internally executes
the replication command ALTER_REPLICATION_SLOT to change the replication slot's
failover property, but this replication command execution cannot be
rollback.
To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
(failover) inside a txn block, which is also consistent to the ALTER
SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
patch to address this.
Best Regards,
Hou Zhijie
Attachments:
v1-0001-Disallow-alter-subscription-s-failover-option-ins.patchapplication/octet-stream; name=v1-0001-Disallow-alter-subscription-s-failover-option-ins.patchDownload
From d792ab2fa2aefb8842774aa44478eebd35851abf Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Date: Tue, 16 Apr 2024 11:31:32 +0800
Subject: [PATCH v1] Disallow alter subscription's failover option inside a
transaction block
Disallow ALTER SUBSCRIPTION in a transaction block when the replication slot is
to be altered, since that cannot be rolled back.
---
src/backend/commands/subscriptioncmds.c | 9 +++++++++
src/test/regress/expected/subscription.out | 5 +++++
src/test/regress/sql/subscription.sql | 5 +++++
3 files changed, 19 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5a47fa984d..9fa5f4e5b4 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1267,6 +1267,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/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 1eee6b17b8..0e6c408be3 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -472,6 +472,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..07413d7e81 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -333,6 +333,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.31.1
Hi,
On Tue, Apr 16, 2024 at 06:32:11AM +0000, Zhijie Hou (Fujitsu) wrote:
Hi,
Kuroda-San reported an issue off-list that:
If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
and rollback, only the subscription option change can be rolled back, while the
replication slot's failover change is preserved.
Nice catch, thanks!
To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
(failover) inside a txn block, which is also consistent to the ALTER
SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
patch to address this.
Agree. The patch looks pretty straightforward to me. Worth to add this
case in the doc? (where we already mention that "Commands ALTER SUBSCRIPTION ...
REFRESH PUBLICATION and ALTER SUBSCRIPTION ... {SET|ADD|DROP} PUBLICATION ...
with refresh option as true cannot be executed inside a transaction block"
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Dear Hou,
Kuroda-San reported an issue off-list that:
If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
and rollback, only the subscription option change can be rolled back, while the
replication slot's failover change is preserved.This is because ALTER SUBSCRIPTION SET (failover) command internally
executes
the replication command ALTER_REPLICATION_SLOT to change the replication
slot's
failover property, but this replication command execution cannot be
rollback.To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
(failover) inside a txn block, which is also consistent to the ALTER
SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
patch to address this.
Thanks for posting the patch, the fix is same as my expectation.
Also, should we add the restriction to the doc? I feel [1]https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET can be updated.
[1]: https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
Dear Hou,
Kuroda-San reported an issue off-list that:
If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
and rollback, only the subscription option change can be rolled back, while the
replication slot's failover change is preserved.This is because ALTER SUBSCRIPTION SET (failover) command internally
executes
the replication command ALTER_REPLICATION_SLOT to change the replication
slot's
failover property, but this replication command execution cannot be
rollback.To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
(failover) inside a txn block, which is also consistent to the ALTER
SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
patch to address this.Thanks for posting the patch, the fix is same as my expectation.
Also, should we add the restriction to the doc? I feel [1] can be updated.
+1.
Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
because we call alter_replication_slot in CREATE SUB as well, for the
case when slot_name is provided and create_slot=false. But the tricky
part is we call alter_replication_slot() when creating subscription
for both failover=true and false. That means if we want to fix it on
the similar line of ALTER SUB, we have to disallow user from executing
the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
to break some existing use cases. (previously, user can execute such a
command inside a txn block).
So, we need to think if there are better ways to fix it. After
discussion with Hou-San offlist, here are some ideas:
1. do not alter replication slot's failover option when CREATE
SUBSCRIPTION WITH failover=false. This means we alter replication
slot only when failover is set to true. And thus we can disallow
CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
inside a txn block.
This option allows user to run CREATE-SUB(create_slot=false) with
failover=false in txn block like earlier. But on the downside, it
makes the behavior inconsistent for otherwise simpler option like
failover, i.e. with failover=true, CREATE SUB is not allowed in txn
block while with failover=false, it is allowed. It makes it slightly
complex to be understood by user.
2. let's not disallow CREATE SUB in txn block as earlier, just don't
perform internal alter-failover during CREATE SUB for existing
slots(create_slot=false, slot_name=xx) i.e. when create_slot is
false, we will ignore failover parameter of CREATE SUB and it is
user's responsibility to set it appropriately using ALTER SUB cmd. For
create_slot=true, the behaviour of CREATE-SUB remains same as earlier.
This option does not add new restriction for CREATE SUB wrt txn block.
In context of failover with create_slot=false, we already have a
similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
failover by himself. CREAT SUB can also be documented in similar way.
This seems simpler to be understood considering existing ALTER SUB's
behavior as well. Plus, this will make CREATE-SUB code slightly
simpler and thus easily manageable.
3. add a alter_slot option for CREATE SUBSCRIPTION, we can only alter
the slot's failover if alter_slot=true. And so we can disallow CREATE
SUB WITH (slot_name =xx, alter_slot=true) inside a txn block.
This seems a clean way, as everything will be as per user's consent
based on alter_slot parameter. But on the downside, this will need
introducing additional parameter and also adding new restriction of
running CREATE-sub in txn block for a specific case.
4. Don't alter replication in subscription DDLs. Instead, try to alter
replication slot's failover in the apply worker. This means we need to
execute alter_replication_slot each time before starting streaming in
the apply worker.
This does not seem appealing to execute alter_replication_slot
everytime the apply worker starts. But if others think it as a better
option, it can be further analyzed.
Currently, our preference is option 2, as that looks a clean solution
and also aligns with ALTER-SUB behavior which is already documented.
Thoughts?
--------------------------------
Note that we could not refer to the design of two_phase here, because
two_phase can be considered as a streaming option, so it's fine to
change the two_phase along with START_REPLICATION command. (the
two_phase is not changed in subscription DDLs, but get changed in
START_REPLICATION command). But the failover is closely related to a
replication slot itself.
--------------------------------
Thanks
Shveta
On Tue, Apr 16, 2024 at 5:06 PM shveta malik <shveta.malik@gmail.com> wrote:
On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:Dear Hou,
Kuroda-San reported an issue off-list that:
If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
and rollback, only the subscription option change can be rolled back, while the
replication slot's failover change is preserved.This is because ALTER SUBSCRIPTION SET (failover) command internally
executes
the replication command ALTER_REPLICATION_SLOT to change the replication
slot's
failover property, but this replication command execution cannot be
rollback.To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
(failover) inside a txn block, which is also consistent to the ALTER
SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
patch to address this.Thanks for posting the patch, the fix is same as my expectation.
Also, should we add the restriction to the doc? I feel [1] can be updated.+1.
Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
because we call alter_replication_slot in CREATE SUB as well, for the
case when slot_name is provided and create_slot=false. But the tricky
part is we call alter_replication_slot() when creating subscription
for both failover=true and false. That means if we want to fix it on
the similar line of ALTER SUB, we have to disallow user from executing
the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
to break some existing use cases. (previously, user can execute such a
command inside a txn block).So, we need to think if there are better ways to fix it. After
discussion with Hou-San offlist, here are some ideas:1. do not alter replication slot's failover option when CREATE
SUBSCRIPTION WITH failover=false. This means we alter replication
slot only when failover is set to true. And thus we can disallow
CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
inside a txn block.This option allows user to run CREATE-SUB(create_slot=false) with
failover=false in txn block like earlier. But on the downside, it
makes the behavior inconsistent for otherwise simpler option like
failover, i.e. with failover=true, CREATE SUB is not allowed in txn
block while with failover=false, it is allowed. It makes it slightly
complex to be understood by user.2. let's not disallow CREATE SUB in txn block as earlier, just don't
perform internal alter-failover during CREATE SUB for existing
slots(create_slot=false, slot_name=xx) i.e. when create_slot is
false, we will ignore failover parameter of CREATE SUB and it is
user's responsibility to set it appropriately using ALTER SUB cmd. For
create_slot=true, the behaviour of CREATE-SUB remains same as earlier.This option does not add new restriction for CREATE SUB wrt txn block.
In context of failover with create_slot=false, we already have a
similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
failover by himself. CREAT SUB can also be documented in similar way.
This seems simpler to be understood considering existing ALTER SUB's
behavior as well. Plus, this will make CREATE-SUB code slightly
simpler and thus easily manageable.
+1 for option 2 as it sounds logical to me and consistent with ALTER
SUBSCRIPTION. BTW, IIUC, you are referring to: "When altering the
slot_name, the failover and two_phase property values of the named
slot may differ from the counterpart failover and two_phase parameters
specified in the subscription. When creating the slot, ensure the slot
properties failover and two_phase match their counterpart parameters
of the subscription." in docs [1], right?
--
With Regards,
Amit Kapila.
On Thu, Apr 18, 2024 at 11:22 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Apr 16, 2024 at 5:06 PM shveta malik <shveta.malik@gmail.com> wrote:
On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:Dear Hou,
Kuroda-San reported an issue off-list that:
If user execute ALTER SUBSCRIPTION SET (failover) command inside a txn block
and rollback, only the subscription option change can be rolled back, while the
replication slot's failover change is preserved.This is because ALTER SUBSCRIPTION SET (failover) command internally
executes
the replication command ALTER_REPLICATION_SLOT to change the replication
slot's
failover property, but this replication command execution cannot be
rollback.To fix it, I think we can prevent user from executing ALTER SUBSCRIPTION set
(failover) inside a txn block, which is also consistent to the ALTER
SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attach a small
patch to address this.Thanks for posting the patch, the fix is same as my expectation.
Also, should we add the restriction to the doc? I feel [1] can be updated.+1.
Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
because we call alter_replication_slot in CREATE SUB as well, for the
case when slot_name is provided and create_slot=false. But the tricky
part is we call alter_replication_slot() when creating subscription
for both failover=true and false. That means if we want to fix it on
the similar line of ALTER SUB, we have to disallow user from executing
the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
to break some existing use cases. (previously, user can execute such a
command inside a txn block).So, we need to think if there are better ways to fix it. After
discussion with Hou-San offlist, here are some ideas:1. do not alter replication slot's failover option when CREATE
SUBSCRIPTION WITH failover=false. This means we alter replication
slot only when failover is set to true. And thus we can disallow
CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
inside a txn block.This option allows user to run CREATE-SUB(create_slot=false) with
failover=false in txn block like earlier. But on the downside, it
makes the behavior inconsistent for otherwise simpler option like
failover, i.e. with failover=true, CREATE SUB is not allowed in txn
block while with failover=false, it is allowed. It makes it slightly
complex to be understood by user.2. let's not disallow CREATE SUB in txn block as earlier, just don't
perform internal alter-failover during CREATE SUB for existing
slots(create_slot=false, slot_name=xx) i.e. when create_slot is
false, we will ignore failover parameter of CREATE SUB and it is
user's responsibility to set it appropriately using ALTER SUB cmd. For
create_slot=true, the behaviour of CREATE-SUB remains same as earlier.This option does not add new restriction for CREATE SUB wrt txn block.
In context of failover with create_slot=false, we already have a
similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
failover by himself. CREAT SUB can also be documented in similar way.
This seems simpler to be understood considering existing ALTER SUB's
behavior as well. Plus, this will make CREATE-SUB code slightly
simpler and thus easily manageable.+1 for option 2 as it sounds logical to me and consistent with ALTER
SUBSCRIPTION. BTW, IIUC, you are referring to: "When altering the
slot_name, the failover and two_phase property values of the named
slot may differ from the counterpart failover and two_phase parameters
specified in the subscription. When creating the slot, ensure the slot
properties failover and two_phase match their counterpart parameters
of the subscription." in docs [1], right?
Yes. Here:
https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET
thanks
Shveta
Dear Shveta,
Sorry for delay response. I missed your post.
+1.
Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
because we call alter_replication_slot in CREATE SUB as well, for the
case when slot_name is provided and create_slot=false. But the tricky
part is we call alter_replication_slot() when creating subscription
for both failover=true and false. That means if we want to fix it on
the similar line of ALTER SUB, we have to disallow user from executing
the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
to break some existing use cases. (previously, user can execute such a
command inside a txn block).So, we need to think if there are better ways to fix it. After
discussion with Hou-San offlist, here are some ideas:
1. do not alter replication slot's failover option when CREATE
SUBSCRIPTION WITH failover=false. This means we alter replication
slot only when failover is set to true. And thus we can disallow
CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
inside a txn block.This option allows user to run CREATE-SUB(create_slot=false) with
failover=false in txn block like earlier. But on the downside, it
makes the behavior inconsistent for otherwise simpler option like
failover, i.e. with failover=true, CREATE SUB is not allowed in txn
block while with failover=false, it is allowed. It makes it slightly
complex to be understood by user.2. let's not disallow CREATE SUB in txn block as earlier, just don't
perform internal alter-failover during CREATE SUB for existing
slots(create_slot=false, slot_name=xx) i.e. when create_slot is
false, we will ignore failover parameter of CREATE SUB and it is
user's responsibility to set it appropriately using ALTER SUB cmd. For
create_slot=true, the behaviour of CREATE-SUB remains same as earlier.This option does not add new restriction for CREATE SUB wrt txn block.
In context of failover with create_slot=false, we already have a
similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
failover by himself. CREAT SUB can also be documented in similar way.
This seems simpler to be understood considering existing ALTER SUB's
behavior as well. Plus, this will make CREATE-SUB code slightly
simpler and thus easily manageable.3. add a alter_slot option for CREATE SUBSCRIPTION, we can only alter
the slot's failover if alter_slot=true. And so we can disallow CREATE
SUB WITH (slot_name =xx, alter_slot=true) inside a txn block.This seems a clean way, as everything will be as per user's consent
based on alter_slot parameter. But on the downside, this will need
introducing additional parameter and also adding new restriction of
running CREATE-sub in txn block for a specific case.4. Don't alter replication in subscription DDLs. Instead, try to alter
replication slot's failover in the apply worker. This means we need to
execute alter_replication_slot each time before starting streaming in
the apply worker.This does not seem appealing to execute alter_replication_slot
everytime the apply worker starts. But if others think it as a better
option, it can be further analyzed.
Thanks for describing, I also prefer 2, because it seems bit strange that
CREATE statement leads ALTER.
Currently, our preference is option 2, as that looks a clean solution
and also aligns with ALTER-SUB behavior which is already documented.
Thoughts?--------------------------------
Note that we could not refer to the design of two_phase here, because
two_phase can be considered as a streaming option, so it's fine to
change the two_phase along with START_REPLICATION command. (the
two_phase is not changed in subscription DDLs, but get changed in
START_REPLICATION command). But the failover is closely related to a
replication slot itself.
--------------------------------
Sorry, I cannot find statements. Where did you refer?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
On Thu, Apr 18, 2024 at 11:40 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
Dear Shveta,
Sorry for delay response. I missed your post.
+1.
Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
because we call alter_replication_slot in CREATE SUB as well, for the
case when slot_name is provided and create_slot=false. But the tricky
part is we call alter_replication_slot() when creating subscription
for both failover=true and false. That means if we want to fix it on
the similar line of ALTER SUB, we have to disallow user from executing
the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
to break some existing use cases. (previously, user can execute such a
command inside a txn block).So, we need to think if there are better ways to fix it. After
discussion with Hou-San offlist, here are some ideas:
1. do not alter replication slot's failover option when CREATE
SUBSCRIPTION WITH failover=false. This means we alter replication
slot only when failover is set to true. And thus we can disallow
CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
inside a txn block.This option allows user to run CREATE-SUB(create_slot=false) with
failover=false in txn block like earlier. But on the downside, it
makes the behavior inconsistent for otherwise simpler option like
failover, i.e. with failover=true, CREATE SUB is not allowed in txn
block while with failover=false, it is allowed. It makes it slightly
complex to be understood by user.2. let's not disallow CREATE SUB in txn block as earlier, just don't
perform internal alter-failover during CREATE SUB for existing
slots(create_slot=false, slot_name=xx) i.e. when create_slot is
false, we will ignore failover parameter of CREATE SUB and it is
user's responsibility to set it appropriately using ALTER SUB cmd. For
create_slot=true, the behaviour of CREATE-SUB remains same as earlier.This option does not add new restriction for CREATE SUB wrt txn block.
In context of failover with create_slot=false, we already have a
similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
failover by himself. CREAT SUB can also be documented in similar way.
This seems simpler to be understood considering existing ALTER SUB's
behavior as well. Plus, this will make CREATE-SUB code slightly
simpler and thus easily manageable.3. add a alter_slot option for CREATE SUBSCRIPTION, we can only alter
the slot's failover if alter_slot=true. And so we can disallow CREATE
SUB WITH (slot_name =xx, alter_slot=true) inside a txn block.This seems a clean way, as everything will be as per user's consent
based on alter_slot parameter. But on the downside, this will need
introducing additional parameter and also adding new restriction of
running CREATE-sub in txn block for a specific case.4. Don't alter replication in subscription DDLs. Instead, try to alter
replication slot's failover in the apply worker. This means we need to
execute alter_replication_slot each time before starting streaming in
the apply worker.This does not seem appealing to execute alter_replication_slot
everytime the apply worker starts. But if others think it as a better
option, it can be further analyzed.Thanks for describing, I also prefer 2, because it seems bit strange that
CREATE statement leads ALTER.
Thanks for feedback.
Currently, our preference is option 2, as that looks a clean solution
and also aligns with ALTER-SUB behavior which is already documented.
Thoughts?--------------------------------
Note that we could not refer to the design of two_phase here, because
two_phase can be considered as a streaming option, so it's fine to
change the two_phase along with START_REPLICATION command. (the
two_phase is not changed in subscription DDLs, but get changed in
START_REPLICATION command). But the failover is closely related to a
replication slot itself.
--------------------------------
Sorry for causing confusion. This is not the statement which is
documented one, this was an additional note to support our analysis.
Sorry, I cannot find statements. Where did you refer?
When I said that option 2 aligns with ALTER-SUB documented behaviour,
I meant the doc described in [1]https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET stating "When altering the slot_name,
the failover and two_phase property values of the named slot may
differ from the counterpart failover and two_phase parameters
specified in the subscription...."
[1]: https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTERSUBSCRIPTION-PARAMS-SET
thanks
Shveta
On Thu, Apr 18, 2024 at 11:22:24AM +0530, Amit Kapila wrote:
+1 for option 2 as it sounds logical to me and consistent with ALTER
SUBSCRIPTION. BTW, IIUC, you are referring to: "When altering the
slot_name, the failover and two_phase property values of the named
slot may differ from the counterpart failover and two_phase parameters
specified in the subscription. When creating the slot, ensure the slot
properties failover and two_phase match their counterpart parameters
of the subscription." in docs [1], right?
FWIW, I'd also favor option 2, mostly on consistency ground as it
would offer a better user-experience. On top of that, you're saying
that may lead to some simplifications in the CREATE path. Without a
patch, it's hard to tell, though.
As far as I can see, this is not tracked as an open item and it should
be. So I have added one.
--
Michael
On Thursday, April 18, 2024 1:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Apr 16, 2024 at 5:06 PM shveta malik <shveta.malik@gmail.com>
wrote:On Tue, Apr 16, 2024 at 1:45 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:Dear Hou,
Kuroda-San reported an issue off-list that:
If user execute ALTER SUBSCRIPTION SET (failover) command inside a
txn block and rollback, only the subscription option change can be
rolled back, while the replication slot's failover change is preserved.This is because ALTER SUBSCRIPTION SET (failover) command
internally executes the replication command ALTER_REPLICATION_SLOT
to change the replication slot's failover property, but this
replication command execution cannot be rollback.To fix it, I think we can prevent user from executing ALTER
SUBSCRIPTION set
(failover) inside a txn block, which is also consistent to the
ALTER SUBSCRIPTION REFRESH/DROP SUBSCRIPTION command. Attacha
small patch to address this.
Thanks for posting the patch, the fix is same as my expectation.
Also, should we add the restriction to the doc? I feel [1] can be updated.+1.
Similar to ALTER SUB, CREATE SUB also needs to be fixed. This is
because we call alter_replication_slot in CREATE SUB as well, for the
case when slot_name is provided and create_slot=false. But the tricky
part is we call alter_replication_slot() when creating subscription
for both failover=true and false. That means if we want to fix it on
the similar line of ALTER SUB, we have to disallow user from executing
the CREATE SUBSCRIPTION (slot_name = xx) in a txn block, which seems
to break some existing use cases. (previously, user can execute such a
command inside a txn block).So, we need to think if there are better ways to fix it. After
discussion with Hou-San offlist, here are some ideas:1. do not alter replication slot's failover option when CREATE
SUBSCRIPTION WITH failover=false. This means we alter replication
slot only when failover is set to true. And thus we can disallow
CREATE SUB WITH (slot_name =xx, failover=true, create_slot=false)
inside a txn block.This option allows user to run CREATE-SUB(create_slot=false) with
failover=false in txn block like earlier. But on the downside, it
makes the behavior inconsistent for otherwise simpler option like
failover, i.e. with failover=true, CREATE SUB is not allowed in txn
block while with failover=false, it is allowed. It makes it slightly
complex to be understood by user.2. let's not disallow CREATE SUB in txn block as earlier, just don't
perform internal alter-failover during CREATE SUB for existing
slots(create_slot=false, slot_name=xx) i.e. when create_slot is
false, we will ignore failover parameter of CREATE SUB and it is
user's responsibility to set it appropriately using ALTER SUB cmd. For
create_slot=true, the behaviour of CREATE-SUB remains same as earlier.This option does not add new restriction for CREATE SUB wrt txn block.
In context of failover with create_slot=false, we already have a
similar restriction (documented one) for ALTER SUB, i.e. with 'ALTER
SUBSCRIPTION SET(slot_name = new)', user needs to alter the new slot's
failover by himself. CREAT SUB can also be documented in similar way.
This seems simpler to be understood considering existing ALTER SUB's
behavior as well. Plus, this will make CREATE-SUB code slightly
simpler and thus easily manageable.+1 for option 2 as it sounds logical to me and consistent with ALTER
SUBSCRIPTION.
+1.
Here is V2 patch which includes the changes for CREATE SUBSCRIPTION as
suggested. Since we don't connect pub to alter slot when (create_slot=false)
anymore, the restriction that disallows failover=true when connect=false is
also removed.
Best Regards,
Hou zj
Attachments:
v2-0001-Fix-the-handling-of-failover-option-in-subscripti.patchapplication/octet-stream; name=v2-0001-Fix-the-handling-of-failover-option-in-subscripti.patchDownload
From 6c6e97b396056ab864ac20bc8112523c583ed450 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
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 <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..bf99d9f7f7 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -183,6 +183,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. When creating the slot, ensure the slot
+ property <literal>failover</literal> 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 <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/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
Dear Shveta,
When I said that option 2 aligns with ALTER-SUB documented behaviour,
I meant the doc described in [1] stating "When altering the slot_name,
the failover and two_phase property values of the named slot may
differ from the counterpart failover and two_phase parameters
specified in the subscription...."[1]:
https://www.postgresql.org/docs/devel/sql-altersubscription.html#SQL-ALTER
SUBSCRIPTION-PARAMS-SET
I see, thanks for the clarification. Agreed that the description is not conflict with
option 2.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
Dear Hou,
Thanks for updating the patch! Let me confirm the content.
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?
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?
Other than that, the patch LGTM.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/
Hi,
On Fri, Apr 19, 2024 at 12:39:40AM +0000, Zhijie Hou (Fujitsu) wrote:
Here is V2 patch which includes the changes for CREATE SUBSCRIPTION as
suggested. Since we don't connect pub to alter slot when (create_slot=false)
anymore, the restriction that disallows failover=true when connect=false is
also removed.
Thanks!
+ specified in the subscription. When creating the slot, ensure the slot
+ property <literal>failover</literal> matches the counterpart parameter
+ of the subscription.The slot could be created before or after the subscription is created, so I think
it needs a bit of rewording (as here it sounds like the sub is already created),
, something like?
"Always ensure the slot property <literal>failover</literal> matches the
counterpart parameter of the subscription and vice versa."
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Fri, Apr 19, 2024 at 6:09 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
Here is V2 patch which includes the changes for CREATE SUBSCRIPTION as
suggested. Since we don't connect pub to alter slot when (create_slot=false)
anymore, the restriction that disallows failover=true when connect=false is
also removed.
Thanks for the patch. I feel getSubscription() also needs to get
'subfailover' option independent of dopt->binary_upgrade i.e. it needs
similar changes as that of dumpSubscription(). I tested pg_dump,
currently it is not dumping failover parameter for failover-enabled
subscriptions, perhaps due to the same bug. Create-sub and Alter-sub
changes look good and work well.
thanks
Shveta
On Friday, April 19, 2024 10:54 AM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> 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]/messages/by-id/CAJpy0uD3YOeDg-tTCUi3EZ8vznRDfDqO_k6LepJpXUV1Z_=gkA@mail.gmail.com and Bertrand[2]/messages/by-id/ZiIxuaiINsuaWuDK@ip-10-97-1-34.eu-west-3.compute.internal's comments.
[1]: /messages/by-id/CAJpy0uD3YOeDg-tTCUi3EZ8vznRDfDqO_k6LepJpXUV1Z_=gkA@mail.gmail.com
[2]: /messages/by-id/ZiIxuaiINsuaWuDK@ip-10-97-1-34.eu-west-3.compute.internal
Best Regards,
Hou zj
Attachments:
v3-0001-Fix-the-handling-of-failover-option-in-subscripti.patchapplication/octet-stream; name=v3-0001-Fix-the-handling-of-failover-option-in-subscripti.patchDownload
From 26a946598acdeb08adaf9ff237c6b4b3a3d61a26 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
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
On Mon, Apr 22, 2024 at 5:57 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
On Friday, April 19, 2024 10:54 AM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> 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.
Tested the patch, works well.
thanks
Shveta
Hi,
On Mon, Apr 22, 2024 at 11:32:14AM +0530, shveta malik wrote:
On Mon, Apr 22, 2024 at 5:57 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s comments.
Thanks!
Tested the patch, works well.
Same here.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Mon, Apr 22, 2024 at 2:31 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
On Mon, Apr 22, 2024 at 11:32:14AM +0530, shveta malik wrote:
On Mon, Apr 22, 2024 at 5:57 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:Attach the V3 patch which also addressed Shveta[1] and Bertrand[2]'s comments.
Thanks!
Tested the patch, works well.
Same here.
Pushed!
--
With Regards,
Amit Kapila.