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

2017-06-03 Thread Dmitry Dolgov 
> On 26 May 2017 at 23:05, Peter Eisentraut <
peter.eisentr...@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?
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 86eb31d..9f7d73c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -941,10 +941,20 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 		res = walrcv_exec(wrconn, cmd.data, 0, NULL);
 
 		if (res->status != WALRCV_OK_COMMAND)
-			ereport(ERROR,
-			(errmsg("could not drop the replication slot \"%s\" on publisher",
-	slotname),
-			 errdetail("The error was: %s", res->err)));
+		{
+			if (res->errcode == ERRCODE_UNDEFINED_OBJECT)
+ereport(ERROR,
+(errmsg("could not drop the replication slot \"%s\" on publisher",
+		slotname),
+ errdetail("The error was: %s", res->err),
+ errhint("Use ALTER SUBSCRIPTION ... SET (slot_name = NONE) "
+		 "to disassociate the subscription from the slot.")));
+			else
+ereport(ERROR,
+(errmsg("could not drop the replication slot \"%s\" on publisher",
+		slotname),
+ errdetail("The error was: %s", res->err)));
+		}
 		else
 			ereport(NOTICE,
 	(errmsg("dropped replication slot \"%s\" on publisher",
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index ebe9c91..1caa051 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -873,6 +873,7 @@ libpqrcv_exec(WalReceiverConn *conn, const char *query,
 {
 	PGresult   *pgres = NULL;
 	WalRcvExecResult *walres = palloc0(sizeof(WalRcvExecResult));
+	const char *errorcode = NULL;
 
 	if (MyDatabaseId == InvalidOid)
 		ereport(ERROR,
@@ -915,6 +916,9 @@ libpqrcv_exec(WalReceiverConn *conn, const char *query,
 		case PGRES_FATAL_ERROR:
 		case PGRES_BAD_RESPONSE:
 			walres->status = WALRCV_ERROR;
+			errorcode = PQresultErrorField(pgres, PG_DIAG_SQLSTATE);
+			walres->errcode = MAKE_SQLSTATE(errorcode[0], errorcode[1], errorcode[2],
+			errorcode[3], errorcode[4]);
 			walres->err = pchomp(PQerrorMessage(conn->streamConn));
 			break;
 	}
diff --git a/src/include/replication/walreceiver.h b/src/include/replication/walreceiver.h
index 31d090c..b0582af 100644
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -186,6 +186,7 @@ typedef struct WalRcvExecResult
 {
 	WalRcvExecStatus status;
 	char	   *err;
+	int			errcode;
 	Tuplestorestate *tuplestore;
 	TupleDesc	tupledesc;
 } WalRcvExecResult;

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

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

2017-06-02 Thread Peter Eisentraut 
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

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

2017-06-01 Thread Petr Jelinek 
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
>>  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

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

2017-05-31 Thread Peter Eisentraut 
On 5/31/17 09:40, Robert Haas wrote:
> On Tue, May 30, 2017 at 3:01 PM, Peter Eisentraut
>  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
From 3d89b959794abe1bd3addeb9c7c1340187a3cef2 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 31 May 2017 22:35:33 -0400
Subject: [PATCH] doc: Add note that DROP SUBSCRIPTION drops replication slot

Add some information about what to do when this fails.
---
 doc/src/sgml/ref/drop_subscription.sgml | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/doc/src/sgml/ref/drop_subscription.sgml 
b/doc/src/sgml/ref/drop_subscription.sgml
index 4f34a35eef..42068d617b 100644
--- a/doc/src/sgml/ref/drop_subscription.sgml
+++ b/doc/src/sgml/ref/drop_subscription.sgml
@@ -74,6 +74,28 @@ Parameters
  
 
  
+  Notes
+
+  
+   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).
+   After that, DROP SUBSCRIPTION will no longer attempt any
+   actions on a remote host.  Note that if the remote replication slot still
+   exists, it should then be dropped manually; otherwise it will continue to
+   reserve WAL and might eventually cause the disk to fill up.  See
+   also .
+  
+ 
+
+ 
   Examples
 
   
-- 
2.13.0


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

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

2017-05-31 Thread Robert Haas 
On Tue, May 30, 2017 at 3:01 PM, Peter Eisentraut
 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

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

2017-05-30 Thread Peter Eisentraut 
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
From 1e09473bb2f571af0a6a8e4ca718d291efa63772 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 30 May 2017 14:57:01 -0400
Subject: [PATCH] Remove replication slot name check from
 ReplicationSlotAcquire()

When trying to access a replication slot that is supposed to already
exist, we don't need to check the naming rules again.  If the slot
does not exist, we will then get a "does not exist" error message, which
is generally more useful from the perspective of an end user.
---
 src/backend/replication/slot.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 5386e86aa6..c0f7fbb2b2 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -331,8 +331,6 @@ ReplicationSlotAcquire(const char *name)
 
Assert(MyReplicationSlot == NULL);
 
-   ReplicationSlotValidateName(name, ERROR);
-
/* Search for the named slot and mark it active if we find it. */
LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
for (i = 0; i < max_replication_slots; i++)
-- 
2.13.0


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

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

2017-05-26 Thread Peter Eisentraut 
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

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

2017-05-25 Thread Petr Jelinek 
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

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

2017-05-25 Thread Peter Eisentraut 
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

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

2017-05-24 Thread Masahiko Sawada 
On Wed, May 24, 2017 at 9:41 PM, Robert Haas  wrote:
> On Wed, May 24, 2017 at 7:31 PM, Peter Eisentraut
>  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, );
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

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

2017-05-24 Thread Robert Haas 
On Wed, May 24, 2017 at 7:31 PM, Peter Eisentraut
 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

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

2017-05-24 Thread Peter Eisentraut 
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

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

2017-05-23 Thread Dmitry Dolgov 
On 23 May 2017 at 07:26, Euler Taveira  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.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 86eb31d..625b5e9 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -39,6 +39,7 @@
 
 #include "replication/logicallauncher.h"
 #include "replication/origin.h"
+#include "replication/slot.h"
 #include "replication/walreceiver.h"
 #include "replication/walsender.h"
 #include "replication/worker_internal.h"
@@ -138,6 +139,9 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
 			*slot_name_given = true;
 			*slot_name = defGetString(defel);
 
+			/* Validate slot_name even if create_slot = false */
+			ReplicationSlotValidateName(*slot_name, ERROR);
+
 			/* Setting slot_name = NONE is treated as no slot name. */
 			if (strcmp(*slot_name, "none") == 0)
 *slot_name = NULL;
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 91ba8ab..6334a18 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -62,6 +62,9 @@ CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpu
 ERROR:  subscription with slot_name = NONE must also set create_slot = false
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
 ERROR:  subscription with slot_name = NONE must also set enabled = false
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = "invalid name", create_slot = false);
+ERROR:  replication slot name "invalid name" contains invalid character
+HINT:  Replication slot names may only contain lower case letters, numbers, and the underscore character.
 -- ok - with slot_name = NONE
 CREATE SUBSCRIPTION testsub3 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, connect = false);
 WARNING:  tables were not subscribed, you will have to run ALTER SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 4b694a3..5b635bc 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -47,6 +47,7 @@ CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpu
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE);
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false);
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = "invalid name", create_slot = false);
 
 -- ok - with slot_name = NONE
 CREATE SUBSCRIPTION testsub3 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, connect = false);

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

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

2017-05-23 Thread Kuntal Ghosh 
On Tue, May 23, 2017 at 10:56 AM, Euler Taveira  wrote:
> 2017-05-22 17:52 GMT-03:00 Dmitry Dolgov <9erthali...@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:  0: 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

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

2017-05-22 Thread Euler Taveira 
2017-05-22 17:52 GMT-03:00 Dmitry Dolgov <9erthali...@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:  0: 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

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

2017-05-22 Thread Dmitry Dolgov 
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:  0: 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?