On Fri, 16 Jul 2021 at 14:06, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Fri, Jul 9, 2021 at 8:20 AM Japin Li <japi...@hotmail.com> wrote:
>>
>> On Thu, 08 Jul 2021 at 18:17, Amit Kapila <amit.kapil...@gmail.com> wrote:
>> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li <japi...@hotmail.com> wrote:
>>
>> Please consider review v3 patch. v3-0001 adds slot_name verification in
>> parse_subscription_options() and comments for why we need disable
>> subscription
>> where set slot_name to NONE.
>>
>
> I think we back-patch this bug-fix till v10 where it was introduced
> and update the comments only in HEAD. So, accordingly, I moved the
> changes into two patches and changed the comments a bit. Can you
> please test the first patch in back-branches? I'll also do it
> separately.
>
I try to back-patch to v10 stable to v14 stable, and attach two new patches:
one for PG10 & PG11 stable, and the other is for PG12 to PG14 stable.
v4 patch can be applied on HEAD. This modify looks good to me.
How do we back-patch to back-branches? I try to use cherry-pick, but it doesn't
always work (without a doubt, it might be some difference between branches).
>> v3-0002 comes from Ranier Vilela, it reduce the
>> overhead strlen in ReplicationSlotValidateName().
>>
>
> I think this patch has nothing to do with this bug-fix, so I suggest
> you discuss this in a separate patch. Personally, I don't think it
> will help in reducing any overhead but there doesn't appear to be any
> harm in changing it as proposed.
I start a new thread to discuss this [1].
[1] -
https://www.postgresql.org/message-id/meyp282mb16696f6dba8ae36a648817b2b6...@meyp282mb1669.ausp282.prod.outlook.com
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
>From 0155ac4fbec0b840ddb46da592b6fb4bc3b85eef Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Fri, 16 Jul 2021 14:37:45 +0800
Subject: [PATCH v5] Don't allow to set replication slot_name as ''.
We don't allow to create replication slot_name as an empty string ('') via
SQL API pg_create_logical_replication_slot() but it is allowed to be set
via Alter Subscription command. This will lead to apply worker repeatedly
keep trying to stream data via slot_name '' and the user is not allowed to
create the slot with that name.
Author: Japin Li
Reviewed-By: Ranier Vilela, Amit Kapila
Backpatch-through: 10, where it was introduced
Discussion: https://postgr.es/m/meyp282mb1669cbd98e721c77ca696499b6...@meyp282mb1669.ausp282.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 3 +++
src/test/regress/expected/subscription.out | 3 +++
src/test/regress/sql/subscription.sql | 3 +++
3 files changed, 9 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f05e86fa93..6d3e870e64 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -39,6 +39,7 @@
#include "replication/logicallauncher.h"
#include "replication/origin.h"
+#include "replication/slot.h"
#include "replication/walreceiver.h"
#include "replication/walsender.h"
#include "replication/worker_internal.h"
@@ -145,6 +146,8 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
/* Setting slot_name = NONE is treated as no slot name. */
if (strcmp(*slot_name, "none") == 0)
*slot_name = NULL;
+ else
+ ReplicationSlotValidateName(*slot_name, ERROR);
}
else if (strcmp(defel->defname, "copy_data") == 0 && copy_data)
{
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 4fcbf7efe9..1bebfb1ade 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -86,6 +86,9 @@ ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 WITH (refresh = fa
ALTER SUBSCRIPTION testsub CONNECTION 'dbname=doesnotexist2';
ALTER SUBSCRIPTION testsub SET (slot_name = 'newname');
-- fail
+ALTER SUBSCRIPTION testsub SET (slot_name = '');
+ERROR: replication slot name "" is too short
+-- fail
ALTER SUBSCRIPTION doesnotexist CONNECTION 'dbname=doesnotexist2';
ERROR: subscription "doesnotexist" does not exist
ALTER SUBSCRIPTION testsub SET (create_slot = false);
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 36fa1bbac8..b20eeb088d 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -65,6 +65,9 @@ ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 WITH (refresh = fa
ALTER SUBSCRIPTION testsub CONNECTION 'dbname=doesnotexist2';
ALTER SUBSCRIPTION testsub SET (slot_name = 'newname');
+-- fail
+ALTER SUBSCRIPTION testsub SET (slot_name = '');
+
-- fail
ALTER SUBSCRIPTION doesnotexist CONNECTION 'dbname=doesnotexist2';
ALTER SUBSCRIPTION testsub SET (create_slot = false);
--
2.30.2
>From c4900ecbeefedaf2c8feaf9d3aa5b827515c2c94 Mon Sep 17 00:00:00 2001
From: Japin Li <japi...@hotmail.com>
Date: Fri, 16 Jul 2021 14:37:45 +0800
Subject: [PATCH v5] Don't allow to set replication slot_name as ''.
We don't allow to create replication slot_name as an empty string ('') via
SQL API pg_create_logical_replication_slot() but it is allowed to be set
via Alter Subscription command. This will lead to apply worker repeatedly
keep trying to stream data via slot_name '' and the user is not allowed to
create the slot with that name.
Author: Japin Li
Reviewed-By: Ranier Vilela, Amit Kapila
Backpatch-through: 10, where it was introduced
Discussion: https://postgr.es/m/meyp282mb1669cbd98e721c77ca696499b6...@meyp282mb1669.ausp282.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 3 +++
src/test/regress/expected/subscription.out | 3 +++
src/test/regress/sql/subscription.sql | 3 +++
3 files changed, 9 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5b02e0de0f..ee255cf683 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -40,6 +40,7 @@
#include "replication/logicallauncher.h"
#include "replication/origin.h"
+#include "replication/slot.h"
#include "replication/walreceiver.h"
#include "replication/walsender.h"
#include "replication/worker_internal.h"
@@ -146,6 +147,8 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
/* Setting slot_name = NONE is treated as no slot name. */
if (strcmp(*slot_name, "none") == 0)
*slot_name = NULL;
+ else
+ ReplicationSlotValidateName(*slot_name, ERROR);
}
else if (strcmp(defel->defname, "copy_data") == 0 && copy_data)
{
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 3ba1e5dcdd..1a7ca156ec 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -86,6 +86,9 @@ ALTER SUBSCRIPTION regress_testsub SET PUBLICATION testpub2, testpub3 WITH (refr
ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist2';
ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'newname');
-- fail
+ALTER SUBSCRIPTION regress_testsub SET (slot_name = '');
+ERROR: replication slot name "" is too short
+-- fail
ALTER SUBSCRIPTION regress_doesnotexist CONNECTION 'dbname=regress_doesnotexist2';
ERROR: subscription "regress_doesnotexist" does not exist
ALTER SUBSCRIPTION regress_testsub SET (create_slot = false);
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 1bc58887f7..277b9290db 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -65,6 +65,9 @@ ALTER SUBSCRIPTION regress_testsub SET PUBLICATION testpub2, testpub3 WITH (refr
ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist2';
ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'newname');
+-- fail
+ALTER SUBSCRIPTION regress_testsub SET (slot_name = '');
+
-- fail
ALTER SUBSCRIPTION regress_doesnotexist CONNECTION 'dbname=regress_doesnotexist2';
ALTER SUBSCRIPTION regress_testsub SET (create_slot = false);
--
2.30.2
>From b139ce3193a0b3600ab9e055485fbf92b4e317c0 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akap...@postgresql.org>
Date: Fri, 16 Jul 2021 10:31:04 +0530
Subject: [PATCH v4 1/2] Don't allow to set replication slot_name as ''.
We don't allow to create replication slot_name as an empty string ('') via
SQL API pg_create_logical_replication_slot() but it is allowed to be set
via Alter Subscription command. This will lead to apply worker repeatedly
keep trying to stream data via slot_name '' and the user is not allowed to
create the slot with that name.
Author: Japin Li
Reviewed-By: Ranier Vilela, Amit Kapila
Backpatch-through: 10, where it was introduced
Discussion: https://postgr.es/m/meyp282mb1669cbd98e721c77ca696499b6...@meyp282mb1669.ausp282.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 2 ++
src/test/regress/expected/subscription.out | 3 +++
src/test/regress/sql/subscription.sql | 3 +++
3 files changed, 8 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5f834a9c30..fb0b0058f2 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -180,6 +180,8 @@ parse_subscription_options(List *stmt_options, bits32 supported_opts, SubOpts *o
/* Setting slot_name = NONE is treated as no slot name. */
if (strcmp(opts->slot_name, "none") == 0)
opts->slot_name = NULL;
+ else
+ ReplicationSlotValidateName(opts->slot_name, ERROR);
}
else if (IsSet(supported_opts, SUBOPT_COPY_DATA) &&
strcmp(defel->defname, "copy_data") == 0)
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index ad6b4e4bd3..67f92b3878 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -86,6 +86,9 @@ ALTER SUBSCRIPTION regress_testsub SET PUBLICATION testpub2, testpub3 WITH (refr
ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist2';
ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'newname');
-- fail
+ALTER SUBSCRIPTION regress_testsub SET (slot_name = '');
+ERROR: replication slot name "" is too short
+-- fail
ALTER SUBSCRIPTION regress_doesnotexist CONNECTION 'dbname=regress_doesnotexist2';
ERROR: subscription "regress_doesnotexist" does not exist
ALTER SUBSCRIPTION regress_testsub SET (create_slot = false);
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index b732871407..88743ab33b 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -65,6 +65,9 @@ ALTER SUBSCRIPTION regress_testsub SET PUBLICATION testpub2, testpub3 WITH (refr
ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist2';
ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'newname');
+-- fail
+ALTER SUBSCRIPTION regress_testsub SET (slot_name = '');
+
-- fail
ALTER SUBSCRIPTION regress_doesnotexist CONNECTION 'dbname=regress_doesnotexist2';
ALTER SUBSCRIPTION regress_testsub SET (create_slot = false);
--
2.28.0.windows.1
>From ee6d81915a9c5fa2160b14439d1d76692cfebaf7 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akap...@postgresql.org>
Date: Fri, 16 Jul 2021 10:55:59 +0530
Subject: [PATCH v4 2/2] Update comments for AlterSubscription.
Add explanation as to why the subscription needs to be disabled to allow
slot_name as none.
Author: Japin Li and Amit Kapila
Discussion: https://postgr.es/m/meyp282mb1669cbd98e721c77ca696499b6...@meyp282mb1669.ausp282.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index fb0b0058f2..41c8397902 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -912,6 +912,13 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
if (IsSet(opts.specified_opts, SUBOPT_SLOT_NAME))
{
+ /*
+ * The subscription must be disabled to allow slot_name as
+ * 'none', otherwise, the apply worker will repeatedly try
+ * to stream the data using that slot_name which neither
+ * exists on the publisher nor the user will be allowed to
+ * create it.
+ */
if (sub->enabled && !opts.slot_name)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
--
2.28.0.windows.1
