Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Mon, 19 Jul 2021 at 17:02, Amit Kapila wrote: > On Fri, Jul 16, 2021 at 2:12 PM Japin Li wrote: >> >> >> On Fri, 16 Jul 2021 at 14:06, Amit Kapila wrote: >> > On Fri, Jul 9, 2021 at 8:20 AM Japin Li wrote: >> >> >> >> On Thu, 08 Jul 2021 at 18:17, Amit Kapila wrote: >> >> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li 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. >> > > The patch you prepared for v14 was not getting applied cleanly, so I > did the required modifications and then pushed. > >> 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). >> > > Yeah, we need to adjust the patch as per the back-branches code. Thanks! -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Fri, Jul 16, 2021 at 2:12 PM Japin Li wrote: > > > On Fri, 16 Jul 2021 at 14:06, Amit Kapila wrote: > > On Fri, Jul 9, 2021 at 8:20 AM Japin Li wrote: > >> > >> On Thu, 08 Jul 2021 at 18:17, Amit Kapila wrote: > >> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li 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. > The patch you prepared for v14 was not getting applied cleanly, so I did the required modifications and then pushed. > 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). > Yeah, we need to adjust the patch as per the back-branches code. -- With Regards, Amit Kapila.
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Fri, 16 Jul 2021 at 14:06, Amit Kapila wrote:
> On Fri, Jul 9, 2021 at 8:20 AM Japin Li wrote:
>>
>> On Thu, 08 Jul 2021 at 18:17, Amit Kapila wrote:
>> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li 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
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
Date: Fri, 16 Jul 2021 14:37:45 +0800
Subject: [PATCH v5] Don't allow to set replication slot_name
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Fri, 16 Jul 2021 at 14:06, Amit Kapila wrote: > On Fri, Jul 9, 2021 at 8:20 AM Japin Li wrote: >> >> On Thu, 08 Jul 2021 at 18:17, Amit Kapila wrote: >> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li 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. > Thanks for your review, I'll test the in back-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. Agreed. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Fri, Jul 9, 2021 at 8:20 AM Japin Li wrote: > > On Thu, 08 Jul 2021 at 18:17, Amit Kapila wrote: > > On Thu, Jul 8, 2021 at 3:43 PM Japin Li 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. > 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. -- With Regards, Amit Kapila. v4-0001-Don-t-allow-to-set-replication-slot_name-as.patch Description: Binary data v4-0002-Update-comments-for-AlterSubscription.patch Description: Binary data
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
Em qui., 8 de jul. de 2021 às 23:50, Japin Li escreveu: > > On Thu, 08 Jul 2021 at 18:17, Amit Kapila wrote: > > On Thu, Jul 8, 2021 at 3:43 PM Japin Li wrote: > >> > >> On Thu, 08 Jul 2021 at 17:51, Amit Kapila > wrote: > >> > On Wed, Jul 7, 2021 at 7:25 PM Japin Li wrote: > >> >> > >> >> Hi, hackers > >> >> > >> >> The documentation [1] says: > >> >> > >> >> When dropping a subscription that is associated with a replication > slot on the > >> >> remote host (the normal state), DROP SUBSCRIPTION will connect to > the remote > >> >> host and try to drop the replication slot as part of its operation. > This is > >> >> necessary so that the resources allocated for the subscription on > the remote > >> >> host are released. If this fails, either because the remote host is > not > >> >> reachable or because the remote replication slot cannot be dropped > or does not > >> >> exist or never existed, the DROP SUBSCRIPTION command will fail. To > proceed in > >> >> this situation, disassociate the subscription from the replication > slot by > >> >> executing ALTER SUBSCRIPTION ... SET (slot_name = NONE). > >> >> > >> >> However, when I try this, it complains the subscription is enabled, > this command > >> >> requires the subscription disabled. Why we need this limitation? > >> >> > >> > > >> > If we don't have this limitation then even after you have set the slot > >> > name to none, the background apply worker corresponding to that > >> > subscription will continue to stream changes via the previous slot. > >> > > >> > >> Yeah, thanks for your explain! Should we add some comments here? > >> > > > > Sure, but let's keep that as a separate HEAD-only patch. > > 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. v3-0002 comes from Ranier Vilela, it reduce > the > overhead strlen in ReplicationSlotValidateName(). > +1 Seems good. regards, Ranier Vilela
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Thu, 08 Jul 2021 at 18:17, Amit Kapila wrote:
> On Thu, Jul 8, 2021 at 3:43 PM Japin Li wrote:
>>
>> On Thu, 08 Jul 2021 at 17:51, Amit Kapila wrote:
>> > On Wed, Jul 7, 2021 at 7:25 PM Japin Li wrote:
>> >>
>> >> Hi, hackers
>> >>
>> >> The documentation [1] says:
>> >>
>> >> When dropping a subscription that is associated with a replication slot
>> >> on the
>> >> remote host (the normal state), DROP SUBSCRIPTION will connect to the
>> >> remote
>> >> host and try to drop the replication slot as part of its operation. This
>> >> is
>> >> necessary so that the resources allocated for the subscription on the
>> >> remote
>> >> host are released. If this fails, either because the remote host is not
>> >> reachable or because the remote replication slot cannot be dropped or
>> >> does not
>> >> exist or never existed, the DROP SUBSCRIPTION command will fail. To
>> >> proceed in
>> >> this situation, disassociate the subscription from the replication slot by
>> >> executing ALTER SUBSCRIPTION ... SET (slot_name = NONE).
>> >>
>> >> However, when I try this, it complains the subscription is enabled, this
>> >> command
>> >> requires the subscription disabled. Why we need this limitation?
>> >>
>> >
>> > If we don't have this limitation then even after you have set the slot
>> > name to none, the background apply worker corresponding to that
>> > subscription will continue to stream changes via the previous slot.
>> >
>>
>> Yeah, thanks for your explain! Should we add some comments here?
>>
>
> Sure, but let's keep that as a separate HEAD-only patch.
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. v3-0002 comes from Ranier Vilela, it reduce the
overhead strlen in ReplicationSlotValidateName().
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index eb88d877a5..3a033b5b8c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -176,6 +176,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)
@@ -835,6 +837,12 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
if (IsSet(opts.specified_opts, SUBOPT_SLOT_NAME))
{
+ /*
+ * We have to disable the subscription, otherwise, the
+ * logical replication launcher will try to start logical
+ * replication apply worker though the new slot_name, which
+ * doesn't exists on publisher.
+ */
if (sub->enabled && !opts.slot_name)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 57f7dd9b0a..4ac7259f3a 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 308c098c14..c9d1bd8948 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);
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 8c18b4ed05..b01d9d3999 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -172,8 +172,9 @@ bool
ReplicationSlotValidateName(const char *name, int elevel)
{
const char *cp;
+ int len = strlen(name);
- if (strlen(name) == 0)
+ if (len == 0)
{
ereport(elevel,
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Thu, Jul 8, 2021 at 3:43 PM Japin Li wrote: > > On Thu, 08 Jul 2021 at 17:51, Amit Kapila wrote: > > On Wed, Jul 7, 2021 at 7:25 PM Japin Li wrote: > >> > >> Hi, hackers > >> > >> The documentation [1] says: > >> > >> When dropping a subscription that is associated with a replication slot on > >> the > >> remote host (the normal state), DROP SUBSCRIPTION will connect to the > >> remote > >> host and try to drop the replication slot as part of its operation. This is > >> necessary so that the resources allocated for the subscription on the > >> remote > >> host are released. If this fails, either because the remote host is not > >> reachable or because the remote replication slot cannot be dropped or does > >> not > >> exist or never existed, the DROP SUBSCRIPTION command will fail. To > >> proceed in > >> this situation, disassociate the subscription from the replication slot by > >> executing ALTER SUBSCRIPTION ... SET (slot_name = NONE). > >> > >> However, when I try this, it complains the subscription is enabled, this > >> command > >> requires the subscription disabled. Why we need this limitation? > >> > > > > If we don't have this limitation then even after you have set the slot > > name to none, the background apply worker corresponding to that > > subscription will continue to stream changes via the previous slot. > > > > Yeah, thanks for your explain! Should we add some comments here? > Sure, but let's keep that as a separate HEAD-only patch. -- With Regards, Amit Kapila.
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Thu, 08 Jul 2021 at 17:51, Amit Kapila wrote:
> On Wed, Jul 7, 2021 at 7:25 PM Japin Li wrote:
>>
>> Hi, hackers
>>
>> The documentation [1] says:
>>
>> When dropping a subscription that is associated with a replication slot on
>> the
>> remote host (the normal state), DROP SUBSCRIPTION will connect to the remote
>> host and try to drop the replication slot as part of its operation. This is
>> necessary so that the resources allocated for the subscription on the remote
>> host are released. If this fails, either because the remote host is not
>> reachable or because the remote replication slot cannot be dropped or does
>> not
>> exist or never existed, the DROP SUBSCRIPTION command will fail. To proceed
>> in
>> this situation, disassociate the subscription from the replication slot by
>> executing ALTER SUBSCRIPTION ... SET (slot_name = NONE).
>>
>> However, when I try this, it complains the subscription is enabled, this
>> command
>> requires the subscription disabled. Why we need this limitation?
>>
>
> If we don't have this limitation then even after you have set the slot
> name to none, the background apply worker corresponding to that
> subscription will continue to stream changes via the previous slot.
>
Yeah, thanks for your explain! Should we add some comments here?
>> OTOH, when I execute ALTER SUBSCRIPTION ... SET (slot_name=''), it doesn't
>> complain. However,
>> SELECT select pg_create_logical_replication_slot('', 'pgoutput') complains
>> slot name is too
>> short. Although, the slot will be created at publisher, and validate the
>> slot name, IMO, we
>> can also validate the slot_name in parse_subscription_options() to get the
>> error early.
>> Attached fixes it. Any thoughts?
>>
>
> Oh, I think this should be fixed. Can anyone else think this to be
> valid behavior?
>
Ranier Vilela provides a v2 patch and reduce the overhead of
ReplicationSlotValidateName() in thread [1].
[1]
https://www.postgresql.org/message-id/caeudqaqltnj1wvmklk8zh27sgjw5ojizgymq28bfj-_5qg1...@mail.gmail.com
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Wed, Jul 7, 2021 at 7:25 PM Japin Li wrote:
>
> Hi, hackers
>
> The documentation [1] says:
>
> When dropping a subscription that is associated with a replication slot on the
> remote host (the normal state), DROP SUBSCRIPTION will connect to the remote
> host and try to drop the replication slot as part of its operation. This is
> necessary so that the resources allocated for the subscription on the remote
> host are released. If this fails, either because the remote host is not
> reachable or because the remote replication slot cannot be dropped or does not
> exist or never existed, the DROP SUBSCRIPTION command will fail. To proceed in
> this situation, disassociate the subscription from the replication slot by
> executing ALTER SUBSCRIPTION ... SET (slot_name = NONE).
>
> However, when I try this, it complains the subscription is enabled, this
> command
> requires the subscription disabled. Why we need this limitation?
>
If we don't have this limitation then even after you have set the slot
name to none, the background apply worker corresponding to that
subscription will continue to stream changes via the previous slot.
> In src/backend/commands/subscriptioncmds.c:
>
>if (IsSet(opts.specified_opts, SUBOPT_SLOT_NAME))
>{
>if (sub->enabled && !opts.slot_name)
>ereport(ERROR,
>
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> errmsg("cannot set %s for enabled
> subscription",
>"slot_name = NONE")));
>
>if (opts.slot_name)
>values[Anum_pg_subscription_subslotname - 1] =
>DirectFunctionCall1(namein,
> CStringGetDatum(opts.slot_name));
>else
>nulls[Anum_pg_subscription_subslotname - 1] = true;
>replaces[Anum_pg_subscription_subslotname - 1] = true;
>}
>
>
> OTOH, when I execute ALTER SUBSCRIPTION ... SET (slot_name=''), it doesn't
> complain. However,
> SELECT select pg_create_logical_replication_slot('', 'pgoutput') complains
> slot name is too
> short. Although, the slot will be created at publisher, and validate the slot
> name, IMO, we
> can also validate the slot_name in parse_subscription_options() to get the
> error early.
> Attached fixes it. Any thoughts?
>
Oh, I think this should be fixed. Can anyone else think this to be
valid behavior?
With Regards,
Amit Kapila.
Re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
On Thu, 08 Jul 2021 at 01:32, Ranier Vilela wrote:
>>OTOH, when I execute ALTER SUBSCRIPTION ... SET (slot_name=''), it doesn't
> complain. However,
>>SELECT select pg_create_logical_replication_slot('', 'pgoutput') complains
> slot name is too
>>short. Although, the slot will be created at publisher, and validate the
> slot name, IMO, we
>>can also validate the slot_name in parse_subscription_options() to get the
> error early.
>>Attached fixes it. Any thoughts?
> I think that this fix is better after the check if the name is equal to
> "none".
> Most of the time it will be "none" .
> While this, reduce the overhead with strlen into
> ReplicationSlotValidateName can it might be worth it.
>
> For convenience, I have attached a new version.
>
Thanks for your review! LGTM.
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
re: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?
>OTOH, when I execute ALTER SUBSCRIPTION ... SET (slot_name=''), it doesn't
complain. However,
>SELECT select pg_create_logical_replication_slot('', 'pgoutput') complains
slot name is too
>short. Although, the slot will be created at publisher, and validate the
slot name, IMO, we
>can also validate the slot_name in parse_subscription_options() to get the
error early.
>Attached fixes it. Any thoughts?
I think that this fix is better after the check if the name is equal to
"none".
Most of the time it will be "none" .
While this, reduce the overhead with strlen into
ReplicationSlotValidateName can it might be worth it.
For convenience, I have attached a new version.
regards,
Ranier Vilela
v2-validate-slot_name-in-parse_subscription_options.patch
Description: Binary data
reduce_overhead_strlen_replicationslotvalidatename.patch
Description: Binary data
