Thread: pg_recvlogical broken in back branches

Hi,

An issue [1] 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

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;

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

On Wed, Apr 25, 2018 at 06:53:38PM -0700, Noah Misch wrote:
> Right.  Done.

Thanks, Noah.
--
Michael