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:
>>
>> On Thu, 08 Jul 2021 at 17:51, Amit Kapila <amit.kapil...@gmail.com> wrote:
>> > On Wed, Jul 7, 2021 at 7:25 PM Japin Li <japi...@hotmail.com> 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,
(errcode(ERRCODE_INVALID_NAME),
@@ -182,7 +183,7 @@ ReplicationSlotValidateName(const char *name, int elevel)
return false;
}
- if (strlen(name) >= NAMEDATALEN)
+ if (len >= NAMEDATALEN)
{
ereport(elevel,
(errcode(ERRCODE_NAME_TOO_LONG),
