Create subscription with `create_slot=false` and incorrect slot name

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
<http://www.timbira.com.br&gt;

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

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

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.

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

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

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

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

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("could not 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

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

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

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

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

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

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]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com 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]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

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

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

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

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

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

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

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

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

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

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?