fix: ensure non-nil rows upon success in simpleQuery

[Yowgf]

Fixes #1059
Fixes #1173

[arp242]

My worry is that there's a real bug in the logic/protocol flow, and that merely setting this to non-nil merely sweeps that under the carpet (possibly returning wrong results).

[Yowgf]

Hmm that makes sense. Maybe some fuzzing tests or other types of tests would capture the error or at least increase our confidence that the current implementation is ok. I would be willing to try to add tests sometime this week.

[arp242]

The Ping() command is just an empty query (;). Normally the server responds with EmptyQueryResponse and then ReadyForQuery to indicate it's ready for another query. Looking at the logic, the only case where it can return (nil, nil) is if the server returns ReadyForQuery without EmptyQueryResponse.

Can reproduce with: master...1059 :

% PQGO_DEBUG=1 go test -run TestXXX
CLIENT → Startup                 69  "\x00\x03\x00\x00client_encoding\x00UTF8\x00datestyle\x00ISO, MDY\x00database\x00pqgo\x00user\x00pqgo\x00\x00"
SERVER ← (Z) ReadyForQuery        1  "I"
         START conn.simpleQuery
CLIENT → (Q) Query                2  ";\x00"
SERVER ← (Z) ReadyForQuery        1  "I"
         END conn.simpleQuery

--- FAIL: TestXXX (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered, repanicked]
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x6981a4]

goroutine 20 [running]:
[..]
github.com/lib/pq.(*rows).Close(0xc0001ce008?)
        /home/martin/code/Golib/pq/rows.go:43 +0x24
github.com/lib/pq.(*conn).Ping(0xc0001ce008, {0x86b638?, 0xadb8e0?})
        /home/martin/code/Golib/pq/conn_go18.go:91 +0xb0
database/sql.(*DB).pingDC.func1()
        /usr/lib/go/src/database/sql/sql.go:886 +0x2d
database/sql.withLock({0x86a620, 0xc00020e080}, 0xc0001a3e30)
        /usr/lib/go/src/database/sql/sql.go:3572 +0x71
database/sql.(*DB).pingDC(0xc000076e98?, {0x86b638?, 0xadb8e0?}, 0xc00020e080?, 0xc0001a3eb0)
        /usr/lib/go/src/database/sql/sql.go:885 +0x91
database/sql.(*DB).PingContext(0xc00013aa90, {0x86b638, 0xadb8e0})
        /usr/lib/go/src/database/sql/sql.go:908 +0xd2
database/sql.(*DB).Ping(...)
        /usr/lib/go/src/database/sql/sql.go:917
github.com/lib/pq.TestXXX(0xc000102c40)
        /home/martin/code/Golib/pq/conn_test.go:2289 +0xbd
testing.tRunner(0xc000102c40, 0x7fc700)
        /usr/lib/go/src/testing/testing.go:1934 +0xea
created by testing.(*T).Run in goroutine 1
        /usr/lib/go/src/testing/testing.go:1997 +0x465

So the question is why this happens.

[Yowgf]

At the time I got that bug I was using this driver against AWS RDS, AWS Redshift and Denodo, because they are all postgres-compatible. I tried out AWS RDS and Redshift today and it didn't trigger an error. I can't try Denodo right now but maybe that's the one that triggered an error... My understanding is that if a server responds to the ';' query with no EmptyQueryResponse, they are not correctly implementing the Postgres protocol v3.0.

In any case, it might be a good idea to make the client implementation defensive -> even if the server decides to return only ReadyForQuery (no EmptyQueryResponse), the client should still not crash.

[arp242]

Fixed via #1234.