pg_recvlogical broken in back branches
Hi,
An issue [1]https://github.com/eulerto/wal2json/issues/61 reported that pg_recvlogical emitted an error in 9.6.8. I
confirmed that it was broken in recent minor versions (9.6.8, 9.5.12,
9.4.17 -- using same server/client version). It was broken by commit
582edc369cdbd348d68441fc50fa26a84afd0c1a and its siblings.
$ postgres --version
postgres (PostgreSQL) 9.6.8
$ pg_recvlogical --version
pg_recvlogical (PostgreSQL) 9.6.8
$ pg_recvlogical -d postgres --slot test_slot --create-slot -P test_decoding
ERRO: syntax error
pg_recvlogical: could not clear search_path: ERRO: syntax error
Replication protocol supports queries since 10 so the code seems correct to it.
* The capacity to run normal SQL queries was added in PostgreSQL
* 10, so the search path cannot be changed (by us or attackers) on
* earlier versions.
It seems version >= 10 should be checked in old clients too. Version
9.6.8 could not connect to 9.4.17.
$ pg_recvlogical --version
pg_recvlogical (PostgreSQL) 9.6.8
$ psql -p 9994 postgres
psql (9.6.8, servidor 9.4.17)
Digite "help" para ajuda.
$ pg_recvlogical -p 9994 -d postgres --slot test_slot --create-slot -P
test_decoding
pg_recvlogical: could not clear search_path: ERRO: syntax error
A proposed fix is attached. It should be applied to 9.4, 9.5, 9.6, and
10. (Although, client version 10 can connect to server version 10,
client version 10 can't connect to server version 9.6.)
Comments?
[1]: https://github.com/eulerto/wal2json/issues/61
--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
Attachments:
recvl.patchtext/x-patch; charset=US-ASCII; name=recvl.patchDownload+7-2
On Tue, Apr 17, 2018 at 03:01:33AM -0300, Euler Taveira wrote:
A proposed fix is attached. It should be applied to 9.4, 9.5, 9.6, and
10. (Although, client version 10 can connect to server version 10,
client version 10 can't connect to server version 9.6.)Comments?
The exact same fix has already applied on all stable branches:
- af5fbb1286 -> REL9_4_STABLE
- 24ff0fe877 -> REL9_5_STABLE
- 59743deca9 -> REL9_6_STABLE
- e7d3a37d99 -> REL_10_STABLE
- 8d2814f274 -> master
Visibly you missed it.
--
Michael
2018-04-17 3:38 GMT-03:00 Michael Paquier <michael@paquier.xyz>:
The exact same fix has already applied on all stable branches:
Sorry about the noise. I've only checked the REL9_6_8 tag and the tarball.
--
Euler Taveira Timbira -
http://www.timbira.com.br/
PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento
On Tue, Apr 17, 2018 at 03:38:13PM +0900, Michael Paquier wrote:
On Tue, Apr 17, 2018 at 03:01:33AM -0300, Euler Taveira wrote:
A proposed fix is attached. It should be applied to 9.4, 9.5, 9.6, and
10. (Although, client version 10 can connect to server version 10,
client version 10 can't connect to server version 9.6.)Comments?
The exact same fix has already applied on all stable branches:
- af5fbb1286 -> REL9_4_STABLE
- 24ff0fe877 -> REL9_5_STABLE
- 59743deca9 -> REL9_6_STABLE
- e7d3a37d99 -> REL_10_STABLE
- 8d2814f274 -> master
That change is testing the wrong variable. I plan to repair it as attached.
I ran check-world with the following and found no similar defects:
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6106,4 +6106,5 @@ int
PQserverVersion(const PGconn *conn)
{
+ Assert(conn);
if (!conn)
return 0;Attachments:
recvlogical-fix-version-test-v1.patchtext/plain; charset=us-asciiDownload+1-1
On Sun, Apr 22, 2018 at 02:55:51PM -0700, Noah Misch wrote:
That change is testing the wrong variable. I plan to repair it as
attached.
You are right here. Ditto
The whole GetConnection() business in src/bin/pg_basebackup is confusing
with the presence of a global variable. I am wondering if we should
change GetConnection() to SetConnection(). Most of the routines of
streamutil.c also use "conn" as a local variable while being defined as
a global variable in streamutil.h, which don't help much.
--
Michael
On Mon, Apr 23, 2018 at 12:49 AM, Michael Paquier <michael@paquier.xyz>
wrote:
On Sun, Apr 22, 2018 at 02:55:51PM -0700, Noah Misch wrote:
That change is testing the wrong variable. I plan to repair it as
attached.You are right here. Ditto
Ouch indeed. +1 for the fix.
Noah, you are planning to push it yourself, right?
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
On Wed, Apr 25, 2018 at 03:57:49PM +0200, Magnus Hagander wrote:
On Mon, Apr 23, 2018 at 12:49 AM, Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Apr 22, 2018 at 02:55:51PM -0700, Noah Misch wrote:
That change is testing the wrong variable. I plan to repair it as
attached.You are right here. Ditto
Ouch indeed. +1 for the fix.
Noah, you are planning to push it yourself, right?
Right. Done.