[Bug fix]If recovery.conf has target_session_attrs=read-write, the standby fails to start.

Started by Higuchi, Daisukeover 8 years ago7 messages

higuchi.daisuke@jp.fujitsu.com

over 8 years ago

1 attachment(s)

Hello,

I found a problem with libpq connection failover.
If primary_conninfo in recovery.conf has 'target_session_attrs=read-write', the standby fails to start.

How to reproduce the bug:
1. Prepare two standby (standby_1, standby_2) for one master.
On standby_1, primary_conninfo in recovery.conf specifies master.
On standby_2, primary_conninfo in recovery.conf specifies master and standby_2 and target_session_attrs=read-write.
For example, the standby_2's recovery.conf setting is as follows.
-----------------------------------------------
standby_mode = 'on'
primary_conninfo = 'host=localhost,localhost port=5432,5433 target_session_attrs=read-write'
recovery_target_timeline = 'latest'
-----------------------------------------------
2. Starts master, standby_1 and standby_2. When try to start standby_2, the following error is output.
-----------------------------------------------
Test "show transaction_read_only" failed on "localhost: 5432"
ERROR: not connected to database
Test "show transaction_read_only" failed on "localhost: 5433"
ERROR: not connected to database
-----------------------------------------------
I wanted to test if standby_2 re-connects to the new master when master is stopped and standby_1 is promoted, but I failed at the start-up stage.

The reason of this problem is that sending 'show transaction_read_only' is failed.
'show' must be in uppercase letters because the streaming replication protocol only accepts capital letters.
In psql and libpq applications that do not use the streaming replication protocol, this error does not occur.

The attached patch fixes this. This patch changes 'show transaction_read_only' to 'SHOW transaction_read_only'.

Regards,
Daisuke, Higuchi

Attachments:

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

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index bf95b0b..d20d314 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1425,7 +1425,7 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
         If this parameter is set to <literal>read-write</literal>, only a
         connection in which read-write transactions are accepted by default
         is considered acceptable.  The query
-        <literal>show transaction_read_only</literal> will be sent upon any
+        <literal>SHOW transaction_read_only</literal> will be sent upon any
         successful connection; if it returns <literal>on</>, the connection
         will be closed.  If multiple hosts were specified in the connection
         string, any remaining servers will be tried just as if the connection
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 02b7358..8e3ef4b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2845,7 +2845,7 @@ keep_going:						/* We will come back to here until there is
 
 					conn->status = CONNECTION_OK;
 					if (!PQsendQuery(conn,
-									 "show transaction_read_only"))
+									 "SHOW transaction_read_only"))
 					{
 						restoreErrorMessage(conn, &savedMessage);
 						goto error_return;
@@ -2901,7 +2901,7 @@ keep_going:						/* We will come back to here until there is
 
 				conn->status = CONNECTION_OK;
 				if (!PQsendQuery(conn,
-								 "show transaction_read_only"))
+								 "SHOW transaction_read_only"))
 				{
 					restoreErrorMessage(conn, &savedMessage);
 					goto error_return;
@@ -3014,14 +3014,14 @@ keep_going:						/* We will come back to here until there is
 				}
 
 				/*
-				 * Something went wrong with "show transaction_read_only". We
+				 * Something went wrong with "SHOW transaction_read_only". We
 				 * should try next addresses.
 				 */
 				if (res)
 					PQclear(res);
 				restoreErrorMessage(conn, &savedMessage);
 				appendPQExpBuffer(&conn->errorMessage,
-				  libpq_gettext("test \"show transaction_read_only\" failed "
+				  libpq_gettext("test \"SHOW transaction_read_only\" failed "
 								" on \"%s:%s\"\n"),
 								  conn->connhost[conn->whichhost].host,
 								  conn->connhost[conn->whichhost].port);

Michael Paquier

michael.paquier@gmail.com

over 8 years ago

In reply to: Higuchi, Daisuke (#1)

Re: [Bug fix]If recovery.conf has target_session_attrs=read-write, the standby fails to start.

On Fri, May 19, 2017 at 1:16 PM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:

The reason of this problem is that sending 'show transaction_read_only' is failed.
'show' must be in uppercase letters because the streaming replication protocol only accepts capital letters.
In psql and libpq applications that do not use the streaming replication protocol, this error does not occur.

The attached patch fixes this. This patch changes 'show transaction_read_only' to 'SHOW transaction_read_only'.

                     if (!PQsendQuery(conn,
-                                     "show transaction_read_only"))
+                                     "SHOW transaction_read_only"))
Or perhaps the replication command parser could be made more flexible
with lower-case characters for replication commands?
-- 
Michael

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

Higuchi, Daisuke

higuchi.daisuke@jp.fujitsu.com

over 8 years ago

In reply to: Michael Paquier (#2)

1 attachment(s)

Re: [Bug fix]If recovery.conf has target_session_attrs=read-write, the standby fails to start.

From: Michael Paquier [mailto:michael.paquier@gmail.com]

if (!PQsendQuery(conn,
-                                     "show transaction_read_only"))
+                                     "SHOW transaction_read_only"))
Or perhaps the replication command parser could be made more flexible with lower-case characters for replication commands?

By adding flex option '-i', replication command parser could be more flexible.

# repl_scanner is compiled as part of repl_gram
repl_gram.o: repl_scanner.c
+repl_scanner.c: FLEXFLAGS = -CF -p -i

This option is already used for syncrep_scanner.c, so it is not strange to add for repl_scanner.c too.
Attached patch also fixes this problem.

Regards,
Daisuke, Higuchi

Attachments:

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

diff --git a/src/backend/replication/Makefile b/src/backend/replication/Makefile
index da8bcf0..2e32788 100644
--- a/src/backend/replication/Makefile
+++ b/src/backend/replication/Makefile
@@ -23,6 +23,7 @@ include $(top_srcdir)/src/backend/common.mk
 
 # repl_scanner is compiled as part of repl_gram
 repl_gram.o: repl_scanner.c
+repl_scanner.c: FLEXFLAGS = -CF -p -i
 
 # syncrep_scanner is complied as part of syncrep_gram
 syncrep_gram.o: syncrep_scanner.c

Robert Haas

robertmhaas@gmail.com

over 8 years ago

In reply to: Michael Paquier (#2)

Re: [Bug fix]If recovery.conf has target_session_attrs=read-write, the standby fails to start.

On Fri, May 19, 2017 at 12:51 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, May 19, 2017 at 1:16 PM, Higuchi, Daisuke
<higuchi.daisuke@jp.fujitsu.com> wrote:

The reason of this problem is that sending 'show transaction_read_only' is failed.
'show' must be in uppercase letters because the streaming replication protocol only accepts capital letters.
In psql and libpq applications that do not use the streaming replication protocol, this error does not occur.

The attached patch fixes this. This patch changes 'show transaction_read_only' to 'SHOW transaction_read_only'.
if (!PQsendQuery(conn,
-                                     "show transaction_read_only"))
+                                     "SHOW transaction_read_only"))
Or perhaps the replication command parser could be made more flexible
with lower-case characters for replication commands?

Maybe, but that sounds a lot more likely to inflict collateral damage
of some kind than the originally-proposed fix, so I've committed that
fix instead.

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

Tom Lane

tgl@sss.pgh.pa.us

over 8 years ago

In reply to: Higuchi, Daisuke (#3)

Re: [Bug fix]If recovery.conf has target_session_attrs=read-write, the standby fails to start.

"Higuchi, Daisuke" <higuchi.daisuke@jp.fujitsu.com> writes:

By adding flex option '-i', replication command parser could be more flexible.
This option is already used for syncrep_scanner.c, so it is not strange to add for repl_scanner.c too.

Really? That wasn't an especially bright idea IMO, because it means that
the scanner's behavior will be dependent on the locale flex was run in.
An example here is that Turkish locale is likely to have a different
idea of what "FIRST" matches than other locales do. Indeed, if I run
the flex build in a non-C locale, I get warnings like

$ LANG=en_US /usr/bin/flex -b -CF -p -i -o'syncrep_scanner.c' syncrep_scanner.l
syncrep_scanner.l:91: warning, the character range [\200-\377] is ambiguous in a case-insensitive scanner
syncrep_scanner.l:91: warning, the character range [\200-\377] is ambiguous in a case-insensitive scanner

We'd probably be better off to implement case-insensitivity the hard way.
There is a reason why none of our other flex scanners use this switch.

While I'm whining ... it looks like the other flex options selected here
were cargo-culted in rather than being thought about. Surely we don't
run syncrep_scanner often enough, nor over so much data, that it's a
good tradeoff to use the options for larger tables.

regards, tom lane

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

Michael Paquier

michael.paquier@gmail.com

over 8 years ago

In reply to: Tom Lane (#5)

Re: [Bug fix]If recovery.conf has target_session_attrs=read-write, the standby fails to start.

On Sat, May 20, 2017 at 5:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

We'd probably be better off to implement case-insensitivity the hard way.
There is a reason why none of our other flex scanners use this switch.

Do you think that it would be better to list the letter list for each
keyword in repl_scanner.l or have something more generic? As not that
many commands are added in the replication protocol, I would think
that this is more than enough for each command, say:
[Ss][Ll][Oo][Tt] { return K_SLOT; }
etc.

While I'm whining ... it looks like the other flex options selected here
were cargo-culted in rather than being thought about. Surely we don't
run syncrep_scanner often enough, nor over so much data, that it's a
good tradeoff to use the options for larger tables.

Indeed, good catch.
--
Michael

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

Tom Lane

tgl@sss.pgh.pa.us

over 8 years ago

In reply to: Michael Paquier (#6)

Re: [Bug fix]If recovery.conf has target_session_attrs=read-write, the standby fails to start.

Michael Paquier <michael.paquier@gmail.com> writes:

Do you think that it would be better to list the letter list for each
keyword in repl_scanner.l or have something more generic? As not that
many commands are added in the replication protocol, I would think
that this is more than enough for each command, say:
[Ss][Ll][Oo][Tt] { return K_SLOT; }
etc.

Yeah, that's probably good enough. It might be worth the trouble to
make out-of-line definitions, as in cubescan.l's handling of NaN and
Infinity for instance, but that's just cosmetic.

regards, tom lane

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