Thread: [HACKERS] Create subscription with `create_slot=false` and incorrect slot name

Hi

Maybe this question was already raised before, but I couldn't find a
discussion. When I'm creating a subscription with `create_slot=false` looks
like it's possible to pass a slot name with invalid characters. In this
particular case both publisher and subscriber were on the same machine:

    =# CREATE SUBSCRIPTION test_sub CONNECTION 'dbname=db host=host port=port user=user' PUBLICATION test_pub WITH (create_slot=false, slot_name='test slot');
    NOTICE:  00000: synchronized table states
    LOCATION:  CreateSubscription, subscriptioncmds.c:443
    CREATE SUBSCRIPTION
    Time: 5.887 ms

Of course `test slot` doesn't exist, because I can't create a slot with
incorrect name. And consequently I can't drop this subscription:

    =# DROP SUBSCRIPTION test_sub;
    ERROR:  XX000: could not drop the replication slot "test slot" on publisher
    DETAIL:  The error was: ERROR:  replication slot name "test slot" contains invalid character
    HINT:  Replication slot names may only contain lower case letters, numbers, and the underscore character.
    LOCATION:  DropSubscription, subscriptioncmds.c:947
    Time: 2.615 ms

    =# CREATE SUBSCRIPTION test_sub CONNECTION 'dbname=db host=host port=port user=user' PUBLICATION test_pub WITH (create_slot=false, slot_name='test slot');
    ERROR:  42710: subscription "test_sub" already exists
    LOCATION:  CreateSubscription, subscriptioncmds.c:344
    Time: 0.492 ms

Is it an issue or I'm doing something wrong?
2017-05-22 17:52 GMT-03:00 Dmitry Dolgov <9erthalion6@gmail.com>:
Maybe this question was already raised before, but I couldn't find a
discussion. When I'm creating a subscription with `create_slot=false` looks
like it's possible to pass a slot name with invalid characters. In this
particular case both publisher and subscriber were on the same machine:

    =# CREATE SUBSCRIPTION test_sub CONNECTION 'dbname=db host=host port=port user=user' PUBLICATION test_pub WITH (create_slot=false, slot_name='test slot');
    NOTICE:  00000: synchronized table states
    LOCATION:  CreateSubscription, subscriptioncmds.c:443
    CREATE SUBSCRIPTION
    Time: 5.887 ms

The command succeed even if slot_name is invalid. That's because slot_name isn't validated. ReplicationSlotValidateName() should be called in parse_subscription_options() to avoid a pilot error. IMHO we should prevent a future error (use of invalid slot name).
 
Of course `test slot` doesn't exist, because I can't create a slot with
incorrect name. And consequently I can't drop this subscription:

    =# DROP SUBSCRIPTION test_sub;
    ERROR:  XX000: could not drop the replication slot "test slot" on publisher
    DETAIL:  The error was: ERROR:  replication slot name "test slot" contains invalid character
    HINT:  Replication slot names may only contain lower case letters, numbers, and the underscore character.
    LOCATION:  DropSubscription, subscriptioncmds.c:947
    Time: 2.615 ms

Indeed you can drop the subscription. There are two details: (i) subscription should be disabled and (ii) slot name can't be set.

bar=# drop subscription sub1;
ERROR:  could not drop the replication slot "does_not_exist" on publisher
DETAIL:  The error was: ERROR:  replication slot "does_not_exist" does not exist
bar=# alter subscription sub1 set (slot_name = NONE);
ERROR:  cannot set slot_name = NONE for enabled subscription
bar=# alter subscription sub1 disable;
ALTER SUBSCRIPTION
bar=# drop subscription sub1;
ERROR:  could not drop the replication slot "does_not_exist" on publisher
DETAIL:  The error was: ERROR:  replication slot "does_not_exist" does not exist
bar=# alter subscription sub1 set (slot_name = NONE);
ALTER SUBSCRIPTION
bar=# drop subscription sub1;
DROP SUBSCRIPTION

Should we add a hint for 'could not drop the replication slot' message?


--
   Euler Taveira                                   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
On Tue, May 23, 2017 at 10:56 AM, Euler Taveira <euler@timbira.com.br> wrote:
> 2017-05-22 17:52 GMT-03:00 Dmitry Dolgov <9erthalion6@gmail.com>:
>>
>> Maybe this question was already raised before, but I couldn't find a
>> discussion. When I'm creating a subscription with `create_slot=false`
>> looks
>> like it's possible to pass a slot name with invalid characters. In this
>> particular case both publisher and subscriber were on the same machine:
>>
>>     =# CREATE SUBSCRIPTION test_sub CONNECTION 'dbname=db host=host
>> port=port user=user' PUBLICATION test_pub WITH (create_slot=false,
>> slot_name='test slot');
>>     NOTICE:  00000: synchronized table states
>>     LOCATION:  CreateSubscription, subscriptioncmds.c:443
>>     CREATE SUBSCRIPTION
>>     Time: 5.887 ms
>>
> The command succeed even if slot_name is invalid. That's because slot_name
> isn't validated. ReplicationSlotValidateName() should be called in
> parse_subscription_options() to avoid a pilot error. IMHO we should prevent
> a future error (use of invalid slot name).
>
+1. Since, slot_name can be provided even when create_slot is set
false, it should be validated as well while creating the subscription.



-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com



On 23 May 2017 at 07:26, Euler Taveira <euler@timbira.com.br> wrote:
>
> ReplicationSlotValidateName() should be called in parse_subscription_options() to avoid a pilot error.
> IMHO we should prevent a future error (use of invalid slot name).

Yes, I see now. I assume this little patch should be enough for that.

Attachment
On 5/23/17 02:33, Kuntal Ghosh wrote:
>> The command succeed even if slot_name is invalid. That's because slot_name
>> isn't validated. ReplicationSlotValidateName() should be called in
>> parse_subscription_options() to avoid a pilot error. IMHO we should prevent
>> a future error (use of invalid slot name).
>>
> +1. Since, slot_name can be provided even when create_slot is set
> false, it should be validated as well while creating the subscription.

This came up in a previous thread.  It is up to the publishing end what
slot names it accepts.  So running the validation locally is incorrect.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



On Wed, May 24, 2017 at 7:31 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/23/17 02:33, Kuntal Ghosh wrote:
>>> The command succeed even if slot_name is invalid. That's because slot_name
>>> isn't validated. ReplicationSlotValidateName() should be called in
>>> parse_subscription_options() to avoid a pilot error. IMHO we should prevent
>>> a future error (use of invalid slot name).
>>>
>> +1. Since, slot_name can be provided even when create_slot is set
>> false, it should be validated as well while creating the subscription.
>
> This came up in a previous thread.  It is up to the publishing end what
> slot names it accepts.  So running the validation locally is incorrect.

That argument seems pretty tenuous; surely both ends are PostgreSQL,
and the rules for valid slot names aren't likely to change very often.

But even if we accept it as true, I still don't like the idea that a
DROP can just fail, especially with no real guidance as to how to fix
things so it doesn't fail.  Ideas:

1. If we didn't create the slot (and have never connected to it?),
don't try to drop it.

2. Emit some kind of a HINT telling people about ALTER SUBSCRIPTION ..
SET (slot_name = NONE).

3. ???

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



On Wed, May 24, 2017 at 9:41 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, May 24, 2017 at 7:31 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 5/23/17 02:33, Kuntal Ghosh wrote:
>>>> The command succeed even if slot_name is invalid. That's because slot_name
>>>> isn't validated. ReplicationSlotValidateName() should be called in
>>>> parse_subscription_options() to avoid a pilot error. IMHO we should prevent
>>>> a future error (use of invalid slot name).
>>>>
>>> +1. Since, slot_name can be provided even when create_slot is set
>>> false, it should be validated as well while creating the subscription.
>>
>> This came up in a previous thread.  It is up to the publishing end what
>> slot names it accepts.  So running the validation locally is incorrect.
>
> That argument seems pretty tenuous; surely both ends are PostgreSQL,
> and the rules for valid slot names aren't likely to change very often.
>
> But even if we accept it as true, I still don't like the idea that a
> DROP can just fail, especially with no real guidance as to how to fix
> things so it doesn't fail.  Ideas:
>
> 1. If we didn't create the slot (and have never connected to it?),
> don't try to drop it.
>
> 2. Emit some kind of a HINT telling people about ALTER SUBSCRIPTION ..
> SET (slot_name = NONE).
>
> 3. ???
>

+1 to #2 idea. We already emit such errhint when connection to the
publisher failed. I think we can do the same thing in this case.

subscriptioncmds.c:L928
   wrconn = walrcv_connect(conninfo, true, subname, &err);   if (wrconn == NULL)       ereport(ERROR,
(errmsg("couldnot connect to publisher when attempting to "                            "drop the replication slot
\"%s\"",slotname),                errdetail("The error was: %s", err),                errhint("Use ALTER SUBSCRIPTION
...SET (slot_name = NONE) "                           "to disassociate the subscription from the
 
slot.")));

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center



On 5/24/17 21:41, Robert Haas wrote:
>> This came up in a previous thread.  It is up to the publishing end what
>> slot names it accepts.  So running the validation locally is incorrect.
> 
> That argument seems pretty tenuous; surely both ends are PostgreSQL,
> and the rules for valid slot names aren't likely to change very often.

Remember that this could be used for upgrades and side-grades, so the
local rules could change or be more restricted in the future or
depending on compilation options.

> But even if we accept it as true, I still don't like the idea that a
> DROP can just fail, especially with no real guidance as to how to fix
> things so it doesn't fail.  Ideas:
> 
> 1. If we didn't create the slot (and have never connected to it?),
> don't try to drop it.

That would conceptually be nice, but it would probably create a bunch of
problems of its own.  For one, we would need an interlock so that the
first $anything that connects to the slot registers it in the local
catalog as "it's mine now".

> 2. Emit some kind of a HINT telling people about ALTER SUBSCRIPTION ..
> SET (slot_name = NONE).

The reported error is just one of many errors that can happen when DROP
SUBSCRIPTION tries to drop the slot (doens't exist, still active, no
permission, etc.).  We don't want to give the hint that is effectively
"just forget about the slot then" for all of them.  So we would need
some way to distinguish "you can't do this right now" from "this would
never work" (400 vs 500 errors).

Another way to fix this particular issue is to not verify the
replication slot name before doing the drop.  After all, if the name is
not valid, then you can also just report that it doesn't exist.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



On 25/05/17 23:26, Peter Eisentraut wrote:
> On 5/24/17 21:41, Robert Haas wrote:
>>> This came up in a previous thread.  It is up to the publishing end what
>>> slot names it accepts.  So running the validation locally is incorrect.
>>
>> That argument seems pretty tenuous; surely both ends are PostgreSQL,
>> and the rules for valid slot names aren't likely to change very often.
> 
> Remember that this could be used for upgrades and side-grades, so the
> local rules could change or be more restricted in the future or
> depending on compilation options.
> 
>> But even if we accept it as true, I still don't like the idea that a
>> DROP can just fail, especially with no real guidance as to how to fix
>> things so it doesn't fail.  Ideas:
>>
>> 1. If we didn't create the slot (and have never connected to it?),
>> don't try to drop it.
> 
> That would conceptually be nice, but it would probably create a bunch of
> problems of its own.  For one, we would need an interlock so that the
> first $anything that connects to the slot registers it in the local
> catalog as "it's mine now".
> 
>> 2. Emit some kind of a HINT telling people about ALTER SUBSCRIPTION ..
>> SET (slot_name = NONE).
> 
> The reported error is just one of many errors that can happen when DROP
> SUBSCRIPTION tries to drop the slot (doens't exist, still active, no
> permission, etc.).  We don't want to give the hint that is effectively
> "just forget about the slot then" for all of them.  So we would need
> some way to distinguish "you can't do this right now" from "this would
> never work" (400 vs 500 errors).
> 

This specific error returns ERRCODE_UNDEFINED_OBJECT error code so we
could check for it and offer hint only for this case.



--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services



On 5/25/17 19:16, Petr Jelinek wrote:
>> The reported error is just one of many errors that can happen when DROP
>> SUBSCRIPTION tries to drop the slot (doens't exist, still active, no
>> permission, etc.).  We don't want to give the hint that is effectively
>> "just forget about the slot then" for all of them.  So we would need
>> some way to distinguish "you can't do this right now" from "this would
>> never work" (400 vs 500 errors).
>>
> This specific error returns ERRCODE_UNDEFINED_OBJECT error code so we
> could check for it and offer hint only for this case.

We would have to extend the walreceiver interface a little to pass that
through, but it seems doable.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



On Fri, May 26, 2017 at 05:05:37PM -0400, Peter Eisentraut wrote:
> On 5/25/17 19:16, Petr Jelinek wrote:
> >> The reported error is just one of many errors that can happen when DROP
> >> SUBSCRIPTION tries to drop the slot (doens't exist, still active, no
> >> permission, etc.).  We don't want to give the hint that is effectively
> >> "just forget about the slot then" for all of them.  So we would need
> >> some way to distinguish "you can't do this right now" from "this would
> >> never work" (400 vs 500 errors).
> >>
> > This specific error returns ERRCODE_UNDEFINED_OBJECT error code so we
> > could check for it and offer hint only for this case.
> 
> We would have to extend the walreceiver interface a little to pass that
> through, but it seems doable.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Peter,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com



On 5/25/17 17:26, Peter Eisentraut wrote:
> Another way to fix this particular issue is to not verify the
> replication slot name before doing the drop.  After all, if the name is
> not valid, then you can also just report that it doesn't exist.

Here is a possible patch along these lines.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment
On 5/29/17 22:26, Noah Misch wrote:
> On Fri, May 26, 2017 at 05:05:37PM -0400, Peter Eisentraut wrote:
>> On 5/25/17 19:16, Petr Jelinek wrote:
>>>> The reported error is just one of many errors that can happen when DROP
>>>> SUBSCRIPTION tries to drop the slot (doens't exist, still active, no
>>>> permission, etc.).  We don't want to give the hint that is effectively
>>>> "just forget about the slot then" for all of them.  So we would need
>>>> some way to distinguish "you can't do this right now" from "this would
>>>> never work" (400 vs 500 errors).
>>>>
>>> This specific error returns ERRCODE_UNDEFINED_OBJECT error code so we
>>> could check for it and offer hint only for this case.
>>
>> We would have to extend the walreceiver interface a little to pass that
>> through, but it seems doable.
> 
> [Action required within three days.  This is a generic notification.]

I have just posted a patch earlier in this thread with a possible
solution.  If people don't like that approach, then we can pursue the
approach with error codes discussed above.  But I would punt that to
PG11 in that case.  I will report back on Friday.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



On Tue, May 30, 2017 at 3:01 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/25/17 17:26, Peter Eisentraut wrote:
>> Another way to fix this particular issue is to not verify the
>> replication slot name before doing the drop.  After all, if the name is
>> not valid, then you can also just report that it doesn't exist.
>
> Here is a possible patch along these lines.

I don't see how this solves the problem.  Don't you still end up with
an error message telling you that you can't drop the subscription, and
no guidance as to how to fix it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: [HACKERS] Create subscription with `create_slot=false` andincorrect slot name

From
Peter Eisentraut
Date:
On 5/31/17 09:40, Robert Haas wrote:
> On Tue, May 30, 2017 at 3:01 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 5/25/17 17:26, Peter Eisentraut wrote:
>>> Another way to fix this particular issue is to not verify the
>>> replication slot name before doing the drop.  After all, if the name is
>>> not valid, then you can also just report that it doesn't exist.
>>
>> Here is a possible patch along these lines.
> 
> I don't see how this solves the problem.  Don't you still end up with
> an error message telling you that you can't drop the subscription, and
> no guidance as to how to fix it?

Well, the idea was to make the error message less cryptic.

But I notice that there is really little documentation about this.  So
how about the attached documentation patch as well?

As mentioned earlier, if we want to do HINT messages, that will be a bit
more involved and probably PG11 material.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment
On 01/06/17 04:44, Peter Eisentraut wrote:
> On 5/31/17 09:40, Robert Haas wrote:
>> On Tue, May 30, 2017 at 3:01 PM, Peter Eisentraut
>> <peter.eisentraut@2ndquadrant.com> wrote:
>>> On 5/25/17 17:26, Peter Eisentraut wrote:
>>>> Another way to fix this particular issue is to not verify the
>>>> replication slot name before doing the drop.  After all, if the name is
>>>> not valid, then you can also just report that it doesn't exist.
>>>
>>> Here is a possible patch along these lines.
>>
>> I don't see how this solves the problem.  Don't you still end up with
>> an error message telling you that you can't drop the subscription, and
>> no guidance as to how to fix it?
> 
> Well, the idea was to make the error message less cryptic.
> 
> But I notice that there is really little documentation about this.  So
> how about the attached documentation patch as well?
> 
> As mentioned earlier, if we want to do HINT messages, that will be a bit
> more involved and probably PG11 material.
> 

I think the combination of those patches is probably good enough
solution for PG10 (I never understood the need for name validation in
ReplicationSlotAcquire() anyway).

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services



Re: [HACKERS] Create subscription with `create_slot=false` andincorrect slot name

From
Peter Eisentraut
Date:
On 6/1/17 08:20, Petr Jelinek wrote:
> I think the combination of those patches is probably good enough
> solution for PG10 (I never understood the need for name validation in
> ReplicationSlotAcquire() anyway).

I have committed these two patches and will close this open item.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



> On 26 May 2017 at 23:05, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> > On 5/25/17 19:16, Petr Jelinek wrote:
> >> The reported error is just one of many errors that can happen when DROP
> >> SUBSCRIPTION tries to drop the slot (doens't exist, still active, no
> >> permission, etc.).  We don't want to give the hint that is effectively
> >> "just forget about the slot then" for all of them.  So we would need
> >> some way to distinguish "you can't do this right now" from "this would
> >> never work" (400 vs 500 errors).
> >>
> > This specific error returns ERRCODE_UNDEFINED_OBJECT error code so we
> > could check for it and offer hint only for this case.
>
> We would have to extend the walreceiver interface a little to pass that
> through, but it seems doable.

Just to make it clear for me. If I understand correctly, it should be more or
less easy to extend it in that way, something like in the attached patch.
Or am I missing something?
Attachment