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

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

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

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

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

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