Server Crashes if try to provide slot_name='none' at the time of creating subscription.

Started by tusharover 8 years ago13 messages

tushar.ahuja@enterprisedb.com

over 8 years ago

Hi,

Server Crashes if we try to provide slot_name='none' at the time of
creating subscription -

postgres=# create subscription sub2 connection 'dbname=postgres
port=5000 user=centos password=f' publication abc with (slot_name='none');
NOTICE: synchronized table states
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

--
regards,tushar
EnterpriseDB https://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

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: tushar (#1)

1 attachment(s)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On Mon, May 15, 2017 at 6:41 PM, tushar <tushar.ahuja@enterprisedb.com> wrote:

Hi,

Server Crashes if we try to provide slot_name='none' at the time of creating
subscription -

postgres=# create subscription sub2 connection 'dbname=postgres port=5000
user=centos password=f' publication abc with (slot_name='none');
NOTICE: synchronized table states
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Thank you for reporting.
I think create_slot and enabled should be set to false forcibly when
slot_name = 'none'. Attached patch fixes it, more test and regression
test case are needed though.

Regards,

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

Attachments:

fix_bug_of_parse_option.patchapplication/octet-stream; name=fix_bug_of_parse_option.patchDownload

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 21ef15f..b5bf16d 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -133,9 +133,18 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
 			*slot_name_given = true;
 			*slot_name = defGetString(defel);
 
-			/* Setting slot_name = NONE is treated as no slot name. */
+			/*
+			 * Setting slot_name = NONE is treated as no slot name, so
+			 * create_slot and enabled are set to false as well if given.
+			 */
 			if (strcmp(*slot_name, "none") == 0)
+			{
 				*slot_name = NULL;
+				if (create_slot)
+					*create_slot = false;
+				if (enabled)
+					*enabled = false;
+			}
 		}
 		else if (strcmp(defel->defname, "copy_data") == 0 && copy_data)
 		{

Kuntal Ghosh

kuntalghosh.2007@gmail.com

over 8 years ago

In reply to: Masahiko Sawada (#2)

1 attachment(s)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On Mon, May 15, 2017 at 4:00 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 15, 2017 at 6:41 PM, tushar <tushar.ahuja@enterprisedb.com> wrote:

Hi,

Server Crashes if we try to provide slot_name='none' at the time of creating
subscription -

postgres=# create subscription sub2 connection 'dbname=postgres port=5000
user=centos password=f' publication abc with (slot_name='none');
NOTICE: synchronized table states
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Thank you for reporting.
I think create_slot and enabled should be set to false forcibly when
slot_name = 'none'. Attached patch fixes it, more test and regression
test case are needed though.

I think the patch doesn't solve the problem completely. For example,
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432 user=edb password=edb' publication abc with
(slot_name='none',create_slot=true);
ERROR: slot_name = NONE and create slot are mutually exclusive options
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432 user=edb password=edb' publication abc with
(create_slot=true,slot_name='none');
ERROR: could not connect to the publisher: could not connect to
server: Connection refused
Is the server running locally and accepting
connections on Unix domain socket "/tmp/.s.PGSQL.5432"?

Changing the order of subscription parameter changes the output. I
think the modifications should be done at the end of the function.
Attached a patch for the same.

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

Attachments:

fix_bug_of_parse_option_v1.patchapplication/x-download; name=fix_bug_of_parse_option_v1.patchDownload

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 21ef15f..58b55bd 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -209,6 +209,12 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
 					 errmsg("slot_name = NONE and create slot are mutually exclusive options")));
+
+		/* No replication slot should be associated with slot_name = NONE. */
+		if (create_slot)
+			*create_slot = false;
+		if (enabled)
+			*enabled = false;
 	}
 }

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Kuntal Ghosh (#3)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On Mon, May 15, 2017 at 8:06 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:

On Mon, May 15, 2017 at 4:00 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 15, 2017 at 6:41 PM, tushar <tushar.ahuja@enterprisedb.com> wrote:

Hi,

Server Crashes if we try to provide slot_name='none' at the time of creating
subscription -

postgres=# create subscription sub2 connection 'dbname=postgres port=5000
user=centos password=f' publication abc with (slot_name='none');
NOTICE: synchronized table states
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Thank you for reporting.
I think create_slot and enabled should be set to false forcibly when
slot_name = 'none'. Attached patch fixes it, more test and regression
test case are needed though.

I think the patch doesn't solve the problem completely. For example,
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432 user=edb password=edb' publication abc with
(slot_name='none',create_slot=true);
ERROR: slot_name = NONE and create slot are mutually exclusive options
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432 user=edb password=edb' publication abc with
(create_slot=true,slot_name='none');
ERROR: could not connect to the publisher: could not connect to
server: Connection refused
Is the server running locally and accepting
connections on Unix domain socket "/tmp/.s.PGSQL.5432"?

Changing the order of subscription parameter changes the output. I
think the modifications should be done at the end of the function.
Attached a patch for the same.

Yes, you're patch fixes it correctly.

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

Kuntal Ghosh

kuntalghosh.2007@gmail.com

over 8 years ago

In reply to: Masahiko Sawada (#4)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On Mon, May 15, 2017 at 4:39 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

While testing with logical replication, I've found that the server
hangs if we create a subscription to the same server. For example,

$ ./pg_ctl -D Test start -o "-p 5432"
$ ./psql -p 5432 postgres -c "CREATE PUBLICATION abc for all tables
with (publish='delete');"
$ ./psql -p 5432 postgres -c "create subscription sub connection
'dbname=postgres port=5432 user=edb password=edb' publication abc with
(slot_name='abcslot');"
NOTICE: synchronized table states
LOG: logical decoding found initial starting point at 0/162DF18
DETAIL: Waiting for transactions (approximately 1) older than 560 to end.

And, it hangs. Is this an expected behavior?

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

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Kuntal Ghosh (#5)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On Mon, May 15, 2017 at 8:22 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:

On Mon, May 15, 2017 at 4:39 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

While testing with logical replication, I've found that the server
hangs if we create a subscription to the same server. For example,

$ ./pg_ctl -D Test start -o "-p 5432"
$ ./psql -p 5432 postgres -c "CREATE PUBLICATION abc for all tables
with (publish='delete');"
$ ./psql -p 5432 postgres -c "create subscription sub connection
'dbname=postgres port=5432 user=edb password=edb' publication abc with
(slot_name='abcslot');"
NOTICE: synchronized table states
LOG: logical decoding found initial starting point at 0/162DF18
DETAIL: Waiting for transactions (approximately 1) older than 560 to end.

And, it hangs. Is this an expected behavior?

I guess this is what we're discussing on [1]/messages/by-id/20170426165954.GK14000@momjian.us.
Petr answered on that,
===
Yes that's result of how logical replication slots work, the transaction
that needs to finish is your transaction. It can be worked around by
creating the slot manually via the SQL interface for example and create
the subscription using WITH (NOCREATE SLOT, SLOT NAME = 'your slot') .
===

[1]: /messages/by-id/20170426165954.GK14000@momjian.us

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

Kuntal Ghosh

kuntalghosh.2007@gmail.com

over 8 years ago

In reply to: Masahiko Sawada (#6)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On Mon, May 15, 2017 at 5:04 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 15, 2017 at 8:22 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:

On Mon, May 15, 2017 at 4:39 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

While testing with logical replication, I've found that the server
hangs if we create a subscription to the same server. For example,

$ ./pg_ctl -D Test start -o "-p 5432"
$ ./psql -p 5432 postgres -c "CREATE PUBLICATION abc for all tables
with (publish='delete');"
$ ./psql -p 5432 postgres -c "create subscription sub connection
'dbname=postgres port=5432 user=edb password=edb' publication abc with
(slot_name='abcslot');"
NOTICE: synchronized table states
LOG: logical decoding found initial starting point at 0/162DF18
DETAIL: Waiting for transactions (approximately 1) older than 560 to end.

And, it hangs. Is this an expected behavior?

I guess this is what we're discussing on [1].
Petr answered on that,
===
Yes that's result of how logical replication slots work, the transaction
that needs to finish is your transaction. It can be worked around by
creating the slot manually via the SQL interface for example and create
the subscription using WITH (NOCREATE SLOT, SLOT NAME = 'your slot') .
===

[1] /messages/by-id/20170426165954.GK14000@momjian.us

Ohh I see. Thank you.

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

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Masahiko Sawada (#4)

1 attachment(s)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On Mon, May 15, 2017 at 8:09 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 15, 2017 at 8:06 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:

On Mon, May 15, 2017 at 4:00 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 15, 2017 at 6:41 PM, tushar <tushar.ahuja@enterprisedb.com> wrote:

Hi,

Server Crashes if we try to provide slot_name='none' at the time of creating
subscription -

postgres=# create subscription sub2 connection 'dbname=postgres port=5000
user=centos password=f' publication abc with (slot_name='none');
NOTICE: synchronized table states
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Thank you for reporting.
I think create_slot and enabled should be set to false forcibly when
slot_name = 'none'. Attached patch fixes it, more test and regression
test case are needed though.

I think the patch doesn't solve the problem completely. For example,
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432 user=edb password=edb' publication abc with
(slot_name='none',create_slot=true);
ERROR: slot_name = NONE and create slot are mutually exclusive options
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432 user=edb password=edb' publication abc with
(create_slot=true,slot_name='none');
ERROR: could not connect to the publisher: could not connect to
server: Connection refused
Is the server running locally and accepting
connections on Unix domain socket "/tmp/.s.PGSQL.5432"?

Changing the order of subscription parameter changes the output. I
think the modifications should be done at the end of the function.
Attached a patch for the same.

Yes, you're patch fixes it correctly.

I've updated Kuntal's patch, added regression test for option
combination and updated documentation.

Regards,

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

Attachments:

fix_bug_of_parse_option_v2.patchapplication/octet-stream; name=fix_bug_of_parse_option_v2.patchDownload

diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index f2da662..3b6999c 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -145,9 +145,10 @@ CREATE SUBSCRIPTION <replaceable class="PARAMETER">subscription_name</replaceabl
           When <literal>slot_name</literal> is set to
           <literal>NONE</literal>, there will be no replication slot
           associated with the subscription.  This can be used if the
-          replication slot will be created later manually.  Such
-          subscriptions must also have both <literal>enabled</literal> and
-          <literal>create_slot</literal> set to <literal>false</literal>.
+          replication slot will be created later manually.  Setting this
+          to <literal>NONE</literal> will change default values of
+          <literal>enabled</literal> and <literal>create_slot</literal>
+          to <literal>false</literal>.          
          </para>
         </listitem>
        </varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 304ac84..bd667bb 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -199,7 +199,7 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
 
 	/*
 	 * Do additional checking for disallowed combination when
-	 * slot_name = NONE was used.
+	 * slot_name = NONE was used, and do additional processing.
 	 */
 	if (slot_name && *slot_name_given && !*slot_name)
 	{
@@ -212,6 +212,12 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
 					 errmsg("slot_name = NONE and create slot are mutually exclusive options")));
+
+		/* Change the defaults of other options. */
+		if (create_slot)
+			*create_slot = false;
+		if (enabled)
+			*enabled = false;
 	}
 }
 
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 10c3644..8a3cb53 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -49,6 +49,26 @@ SET SESSION AUTHORIZATION 'regress_subscription_user';
 ALTER SUBSCRIPTION testsub CONNECTION 'foobar';
 ERROR:  invalid connection string syntax: missing "=" after "foobar" in connection info string
 
+-- fail - invalid option combination
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, copy_data = true);
+ERROR:  noconnect and copy data are mutually exclusive options
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, enabled = true);
+ERROR:  noconnect and enabled are mutually exclusive options
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true);
+ERROR:  noconnect and create slot are mutually exclusive options
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
+ERROR:  slot_name = NONE and enabled are mutually exclusive options
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
+ERROR:  slot_name = NONE and create slot are mutually exclusive options
+-- ok - with slot_name = NONE
+CREATE SUBSCRIPTION nonesub CONNECTION 'dbname=doesnotexits' 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
+-- fail
+ALTER SUBSCRIPTION nonesub ENABLE;
+ERROR:  cannot enable subscription that does not have a slot name
+ALTER SUBSCRIPTION nonesub REFRESH PUBLICATION;
+ERROR:  ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions
+DROP SUBSCRIPTION nonesub;
 \dRs+
                                          List of subscriptions
   Name   |           Owner           | Enabled | Publication | Synchronous commit |      Conninfo       
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 798bb0d..29fa462 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -41,6 +41,21 @@ SET SESSION AUTHORIZATION 'regress_subscription_user';
 -- fail - invalid connection string
 ALTER SUBSCRIPTION testsub CONNECTION 'foobar';
 
+-- fail - invalid option combination
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, copy_data = true);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, enabled = true);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
+
+-- ok - with slot_name = NONE
+CREATE SUBSCRIPTION nonesub CONNECTION 'dbname=doesnotexits' PUBLICATION testpub WITH (slot_name = NONE, connect = false);
+-- fail
+ALTER SUBSCRIPTION nonesub ENABLE;
+ALTER SUBSCRIPTION nonesub REFRESH PUBLICATION;
+
+DROP SUBSCRIPTION nonesub;
+
 \dRs+
 
 ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 SKIP REFRESH;

Kuntal Ghosh

kuntalghosh.2007@gmail.com

over 8 years ago

In reply to: Masahiko Sawada (#8)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

Added to open item lists.

On Tue, May 16, 2017 at 6:35 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 15, 2017 at 8:09 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 15, 2017 at 8:06 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:

On Mon, May 15, 2017 at 4:00 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 15, 2017 at 6:41 PM, tushar <tushar.ahuja@enterprisedb.com> wrote:

Hi,

Server Crashes if we try to provide slot_name='none' at the time of creating
subscription -

postgres=# create subscription sub2 connection 'dbname=postgres port=5000
user=centos password=f' publication abc with (slot_name='none');
NOTICE: synchronized table states
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Thank you for reporting.
I think create_slot and enabled should be set to false forcibly when
slot_name = 'none'. Attached patch fixes it, more test and regression
test case are needed though.

I think the patch doesn't solve the problem completely. For example,
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432 user=edb password=edb' publication abc with
(slot_name='none',create_slot=true);
ERROR: slot_name = NONE and create slot are mutually exclusive options
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432 user=edb password=edb' publication abc with
(create_slot=true,slot_name='none');
ERROR: could not connect to the publisher: could not connect to
server: Connection refused
Is the server running locally and accepting
connections on Unix domain socket "/tmp/.s.PGSQL.5432"?

Changing the order of subscription parameter changes the output. I
think the modifications should be done at the end of the function.
Attached a patch for the same.

Yes, you're patch fixes it correctly.

I've updated Kuntal's patch, added regression test for option
combination and updated documentation.

Regards,

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

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

#10

tushar

tushar.ahuja@enterprisedb.com

over 8 years ago

In reply to: Masahiko Sawada (#8)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On 05/16/2017 06:35 AM, Masahiko Sawada wrote:

I've updated Kuntal's patch, added regression test for option
combination and updated documentation.

While testing the patch - I found that after dump/restore , we are
getting an error in the log file once we enable the subscription

\\create subscription

postgres=# CREATE SUBSCRIPTION m1 CONNECTION 'dbname=postgres port=5000
' PUBLICATION qdd WITH (slot_name='none');
NOTICE: synchronized table states
CREATE SUBSCRIPTION

\\take the dump
[centos@centos-cpula bin]$ ./pg_dump -Fp -p 9000 postgres > /tmp/d.c
\\check the syntax
[centos@centos-cpula bin]$ cat /tmp/d.c |grep 'create subsc*' -i
CREATE SUBSCRIPTION m1 CONNECTION 'dbname=postgres port=5000 '
PUBLICATION qdd WITH (connect = false, slot_name = '');
\\execute this same syntax against a new database
postgres=# create database test;
CREATE DATABASE
postgres=# \c test
You are now connected to database "test" as user "centos".
test=# CREATE SUBSCRIPTION m1 CONNECTION 'dbname=postgres port=5000 '
PUBLICATION qdd WITH (connect = false, slot_name = '');
WARNING: tables were not subscribed, you will have to run ALTER
SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables
CREATE SUBSCRIPTION

test=# alter subscription m1 refresh publication ;
ERROR: ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled
subscriptions
test=# alter subscription m1 enable ;
ALTER SUBSCRIPTION

Check the message in log file

017-05-16 14:04:48.373 BST [18219] LOG: logical replication apply for
subscription m1 started
2017-05-16 14:04:48.381 BST [18219] ERROR: could not start WAL
streaming: ERROR: replication slot name "" is too short
2017-05-16 14:04:48.382 BST [17843] LOG: worker process: logical
replication worker for subscription 16386 (PID 18219) exited with exit
code 1
2017-05-16 14:04:53.388 BST [17850] LOG: starting logical replication
worker for subscription "m1"
2017-05-16 14:04:53.396 BST [18224] LOG: logical replication apply for
subscription m1 started
2017-05-16 14:04:53.403 BST [18224] ERROR: could not start WAL
streaming: ERROR: replication slot name "" is too short

Is this error message (ERROR: replication slot name "" is too short )
is expected now ?

--
regards,tushar
EnterpriseDB https://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

#11

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: tushar (#10)

2 attachment(s)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On Tue, May 16, 2017 at 10:06 PM, tushar <tushar.ahuja@enterprisedb.com> wrote:

On 05/16/2017 06:35 AM, Masahiko Sawada wrote:

I've updated Kuntal's patch, added regression test for option
combination and updated documentation.

While testing the patch - I found that after dump/restore , we are getting
an error in the log file once we enable the subscription

\\create subscription

postgres=# CREATE SUBSCRIPTION m1 CONNECTION 'dbname=postgres port=5000 '
PUBLICATION qdd WITH (slot_name='none');
NOTICE: synchronized table states
CREATE SUBSCRIPTION

\\take the dump
[centos@centos-cpula bin]$ ./pg_dump -Fp -p 9000 postgres > /tmp/d.c
\\check the syntax
[centos@centos-cpula bin]$ cat /tmp/d.c |grep 'create subsc*' -i
CREATE SUBSCRIPTION m1 CONNECTION 'dbname=postgres port=5000 ' PUBLICATION
qdd WITH (connect = false, slot_name = '');
\\execute this same syntax against a new database
postgres=# create database test;
CREATE DATABASE
postgres=# \c test
You are now connected to database "test" as user "centos".
test=# CREATE SUBSCRIPTION m1 CONNECTION 'dbname=postgres port=5000 '
PUBLICATION qdd WITH (connect = false, slot_name = '');
WARNING: tables were not subscribed, you will have to run ALTER
SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables
CREATE SUBSCRIPTION

test=# alter subscription m1 refresh publication ;
ERROR: ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled
subscriptions
test=# alter subscription m1 enable ;
ALTER SUBSCRIPTION

Check the message in log file

017-05-16 14:04:48.373 BST [18219] LOG: logical replication apply for
subscription m1 started
2017-05-16 14:04:48.381 BST [18219] ERROR: could not start WAL streaming:
ERROR: replication slot name "" is too short
2017-05-16 14:04:48.382 BST [17843] LOG: worker process: logical
replication worker for subscription 16386 (PID 18219) exited with exit code
1
2017-05-16 14:04:53.388 BST [17850] LOG: starting logical replication
worker for subscription "m1"
2017-05-16 14:04:53.396 BST [18224] LOG: logical replication apply for
subscription m1 started
2017-05-16 14:04:53.403 BST [18224] ERROR: could not start WAL streaming:
ERROR: replication slot name "" is too short

Is this error message (ERROR: replication slot name "" is too short ) is
expected now ?

I think there are two bugs; pg_dump should dump slot_name = NONE
instead of '' and subscription should not be created if given slot
name is invalid. The validation check for replication slot name is
done when creating it actually but I think it's more safer to check
when CREATE SUBSCRIPTION. The bug related to setting slot_name = NONE
should be fixed by attached 001 patch, and 002 patch prevents to be
specified invalid replication slot name when CREATE SUBSCRIPTION and
ALTER SUBSCRIPTION SET.

Regards,

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

Attachments:

001_fix_bug_of_slot_name_none_v3.patchapplication/octet-stream; name=001_fix_bug_of_slot_name_none_v3.patchDownload

diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 2c91eb6..b3a493f 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -145,9 +145,10 @@ CREATE SUBSCRIPTION <replaceable class="PARAMETER">subscription_name</replaceabl
           When <literal>slot_name</literal> is set to
           <literal>NONE</literal>, there will be no replication slot
           associated with the subscription.  This can be used if the
-          replication slot will be created later manually.  Such
-          subscriptions must also have both <literal>enabled</literal> and
-          <literal>create_slot</literal> set to <literal>false</literal>.
+          replication slot will be created later manually.  Setting this
+          to <literal>NONE</literal> will change default values of
+          <literal>enabled</literal> and <literal>create_slot</literal>
+          to <literal>false</literal>.          
          </para>
         </listitem>
        </varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 304ac84..bd667bb 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -199,7 +199,7 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
 
 	/*
 	 * Do additional checking for disallowed combination when
-	 * slot_name = NONE was used.
+	 * slot_name = NONE was used, and do additional processing.
 	 */
 	if (slot_name && *slot_name_given && !*slot_name)
 	{
@@ -212,6 +212,12 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
 					 errmsg("slot_name = NONE and create slot are mutually exclusive options")));
+
+		/* Change the defaults of other options. */
+		if (create_slot)
+			*create_slot = false;
+		if (enabled)
+			*enabled = false;
 	}
 }
 
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index f7b2840..9085665 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -3832,7 +3832,10 @@ dumpSubscription(Archive *fout, SubscriptionInfo *subinfo)
 	}
 
 	appendPQExpBuffer(query, " PUBLICATION %s WITH (connect = false, slot_name = ", publications->data);
-	appendStringLiteralAH(query, subinfo->subslotname, fout);
+	if (subinfo->subslotname[0] == '\0')
+		appendPQExpBufferStr(query, "NONE");
+	else
+		appendStringLiteralAH(query, subinfo->subslotname, fout);
 
 	if (strcmp(subinfo->subsynccommit, "off") != 0)
 		appendPQExpBuffer(query, ", synchronous_commit = %s", fmtId(subinfo->subsynccommit));
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 10c3644..8a3cb53 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -49,6 +49,26 @@ SET SESSION AUTHORIZATION 'regress_subscription_user';
 ALTER SUBSCRIPTION testsub CONNECTION 'foobar';
 ERROR:  invalid connection string syntax: missing "=" after "foobar" in connection info string
 
+-- fail - invalid option combination
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, copy_data = true);
+ERROR:  noconnect and copy data are mutually exclusive options
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, enabled = true);
+ERROR:  noconnect and enabled are mutually exclusive options
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true);
+ERROR:  noconnect and create slot are mutually exclusive options
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
+ERROR:  slot_name = NONE and enabled are mutually exclusive options
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
+ERROR:  slot_name = NONE and create slot are mutually exclusive options
+-- ok - with slot_name = NONE
+CREATE SUBSCRIPTION nonesub CONNECTION 'dbname=doesnotexits' 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
+-- fail
+ALTER SUBSCRIPTION nonesub ENABLE;
+ERROR:  cannot enable subscription that does not have a slot name
+ALTER SUBSCRIPTION nonesub REFRESH PUBLICATION;
+ERROR:  ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions
+DROP SUBSCRIPTION nonesub;
 \dRs+
                                          List of subscriptions
   Name   |           Owner           | Enabled | Publication | Synchronous commit |      Conninfo       
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 798bb0d..29fa462 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -41,6 +41,21 @@ SET SESSION AUTHORIZATION 'regress_subscription_user';
 -- fail - invalid connection string
 ALTER SUBSCRIPTION testsub CONNECTION 'foobar';
 
+-- fail - invalid option combination
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, copy_data = true);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, enabled = true);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
+
+-- ok - with slot_name = NONE
+CREATE SUBSCRIPTION nonesub CONNECTION 'dbname=doesnotexits' PUBLICATION testpub WITH (slot_name = NONE, connect = false);
+-- fail
+ALTER SUBSCRIPTION nonesub ENABLE;
+ALTER SUBSCRIPTION nonesub REFRESH PUBLICATION;
+
+DROP SUBSCRIPTION nonesub;
+
 \dRs+
 
 ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 SKIP REFRESH;

002_disallow_invalid_slot_name.patchapplication/octet-stream; name=002_disallow_invalid_slot_name.patchDownload

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 304ac84..29e6da5 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -37,6 +37,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"
@@ -316,6 +317,9 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 (errmsg("must be superuser to create subscriptions"))));
 
+	if (slotname_given)
+		ReplicationSlotValidateName(slotname, ERROR);
+
 	rel = heap_open(SubscriptionRelationId, RowExclusiveLock);
 
 	/* Check if name is used */
@@ -630,6 +634,8 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
 								(errcode(ERRCODE_SYNTAX_ERROR),
 								 errmsg("cannot set slot_name = NONE for enabled subscription")));
 
+					ReplicationSlotValidateName(slotname, ERROR);
+
 					if (slotname)
 						values[Anum_pg_subscription_subslotname - 1] =
 						DirectFunctionCall1(namein, CStringGetDatum(slotname));

#12

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 8 years ago

In reply to: Masahiko Sawada (#11)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On 5/16/17 22:21, Masahiko Sawada wrote:

I think there are two bugs; pg_dump should dump slot_name = NONE
instead of '' and subscription should not be created if given slot
name is invalid. The validation check for replication slot name is
done when creating it actually but I think it's more safer to check
when CREATE SUBSCRIPTION. The bug related to setting slot_name = NONE
should be fixed by attached 001 patch, and 002 patch prevents to be
specified invalid replication slot name when CREATE SUBSCRIPTION and
ALTER SUBSCRIPTION SET.

I have worked through these issues and came up with slightly different
patches.

The issue in pg_dump should have been checking PQgetisnull() before
reading the slot name. That is now fixed.

The issue with slot_name = NONE causing a crash was fixed by adding
additional error checking. I did not change it so that slot_name = NONE
would change the defaults of enabled and create_slot. I think it's
confusing if options have dependencies like that. In any case, creating
a subscription with slot_name = NONE is probably not useful anyway, so
as long as it doesn't crash, we don't need to make it excessively
user-friendly.

I don't think the subscription side should check the validity of the
replication slot name. That is the responsibility of the publication
side. The rules may well be different if you replicate between
different versions or different build options. This is currently
working correctly: If the publication side doesn't like the slot you
specify, either because the name is invalid or the slot doesn't exist,
you get an appropriate error message.

Please check whether everything is working OK for you now. I think this
open item is closed now.

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

#13

Masahiko Sawada

sawada.mshk@gmail.com

over 8 years ago

In reply to: Peter Eisentraut (#12)

Re: Server Crashes if try to provide slot_name='none' at the time of creating subscription.

On Thu, May 18, 2017 at 10:33 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

On 5/16/17 22:21, Masahiko Sawada wrote:

I think there are two bugs; pg_dump should dump slot_name = NONE
instead of '' and subscription should not be created if given slot
name is invalid. The validation check for replication slot name is
done when creating it actually but I think it's more safer to check
when CREATE SUBSCRIPTION. The bug related to setting slot_name = NONE
should be fixed by attached 001 patch, and 002 patch prevents to be
specified invalid replication slot name when CREATE SUBSCRIPTION and
ALTER SUBSCRIPTION SET.

I have worked through these issues and came up with slightly different
patches.

The issue in pg_dump should have been checking PQgetisnull() before
reading the slot name. That is now fixed.

Agreed.

The issue with slot_name = NONE causing a crash was fixed by adding
additional error checking. I did not change it so that slot_name = NONE
would change the defaults of enabled and create_slot. I think it's
confusing if options have dependencies like that. In any case, creating
a subscription with slot_name = NONE is probably not useful anyway, so
as long as it doesn't crash, we don't need to make it excessively
user-friendly.

Also agreed.

I don't think the subscription side should check the validity of the
replication slot name. That is the responsibility of the publication
side. The rules may well be different if you replicate between
different versions or different build options. This is currently
working correctly: If the publication side doesn't like the slot you
specify, either because the name is invalid or the slot doesn't exist,
you get an appropriate error message.

Yeah, now I understood. Agreed.

Please check whether everything is working OK for you now. I think this
open item is closed now.

I've checked these changes, everything is working fine. Thank you!

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