Thread: Why ALTER SUBSCRIPTION ... SET (slot_name='none') requires subscription disabled?

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?

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?

[1] https://www.postgresql.org/docs/current/sql-dropsubscription.html


-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, 08 Jul 2021 at 01:32, Ranier Vilela <ranier.vf@gmail.com> 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.

On Wed, Jul 7, 2021 at 7:25 PM Japin Li <japinli@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.

> 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.

On Thu, 08 Jul 2021 at 17:51, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Jul 7, 2021 at 7:25 PM Japin Li <japinli@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?

>> 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-_5QG1G+A@mail.gmail.com

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, Jul 8, 2021 at 3:43 PM Japin Li <japinli@hotmail.com> wrote:
>
> On Thu, 08 Jul 2021 at 17:51, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Wed, Jul 7, 2021 at 7:25 PM Japin Li <japinli@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.

-- 
With Regards,
Amit Kapila.

On Thu, 08 Jul 2021 at 18:17, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Jul 8, 2021 at 3:43 PM Japin Li <japinli@hotmail.com> wrote:
>>
>> On Thu, 08 Jul 2021 at 17:51, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > On Wed, Jul 7, 2021 at 7:25 PM Japin Li <japinli@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.

On Fri, Jul 9, 2021 at 8:20 AM Japin Li <japinli@hotmail.com> wrote:
>
> On Thu, 08 Jul 2021 at 18:17, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li <japinli@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.

> 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.

On Fri, 16 Jul 2021 at 14:06, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Jul 9, 2021 at 8:20 AM Japin Li <japinli@hotmail.com> wrote:
>>
>> On Thu, 08 Jul 2021 at 18:17, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li <japinli@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.
>

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.

On Fri, 16 Jul 2021 at 14:06, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Jul 9, 2021 at 8:20 AM Japin Li <japinli@hotmail.com> wrote:
>>
>> On Thu, 08 Jul 2021 at 18:17, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li <japinli@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/MEYP282MB16696F6DBA8AE36A648817B2B6119@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Fri, Jul 16, 2021 at 2:12 PM Japin Li <japinli@hotmail.com> wrote:
>
>
> On Fri, 16 Jul 2021 at 14:06, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Fri, Jul 9, 2021 at 8:20 AM Japin Li <japinli@hotmail.com> wrote:
> >>
> >> On Thu, 08 Jul 2021 at 18:17, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li <japinli@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.
>

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.

On Mon, 19 Jul 2021 at 17:02, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Jul 16, 2021 at 2:12 PM Japin Li <japinli@hotmail.com> wrote:
>>
>>
>> On Fri, 16 Jul 2021 at 14:06, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > On Fri, Jul 9, 2021 at 8:20 AM Japin Li <japinli@hotmail.com> wrote:
>> >>
>> >> On Thu, 08 Jul 2021 at 18:17, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >> > On Thu, Jul 8, 2021 at 3:43 PM Japin Li <japinli@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.
>>
>
> 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.