Fix pgconn cancel query through the PGBouncer

[levakin]

Closes #1764

[jackc]

As far as I can tell, the current behavior is still within acceptable behavior for PostgreSQL. The change this reverts fails on Windows. I'm not sure exactly why. @drakkan worked on this change. Perhaps they could elaborate.

But it is surprising that not reading could be a problem -- there are no bytes to read. The only reason the code originally read was to wait for the server to close the connection.

[levakin]

pgconn_test.go:2253: 
        	Error Trace:	D:\a\pgx\pgx\pgconn\pgconn_test.go:2253
        	Error:      	Received unexpected error:
        	            	read tcp [::1]:56732->[::1]:5432: wsarecv: An existing connection was forcibly closed by the remote host.
        	Test:       	TestConnCancelRequest

@drakkan network seems to work differently on windows. Any ideas why?

[drakkan]

Hi,

This change was made to fix test cases on Windows. I thought the error was related to the raw system calls used at the time, but I see this happening even now that we no longer have platform-specific code

[levakin]

hi, @jackc I've added handling of of this specific error. What do you think of it? Can you please run workflow again?

[levakin]

@jackc

The only reason the code originally read was to wait for the server to close the connection.

That's exactly the reason I want to revert this commit.
My current understanding is that due to the usage of PGbouncer, the connection is not fast enough to deliver the instruction and defer cancelConn.Close() closes the connection earlier than the instruction is delivered. And the query keeps running as a result.

Here's the pgbouncer log:

kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:51.238 UTC [1] LOG S-0x5631ef4891d0: shard1/[email protected]:5432 new connection to server (from 172.22.0.6:33088)
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.005 UTC [1] LOG C-0x5631ef481e80: shard1/[email protected]:57294 closing because: client close request (age=345s)
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.005 UTC [1] LOG S-0x5631ef4891d0: shard1/[email protected]:5432 closing because: client disconnect while server was not ready (age=2s)
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.005 UTC [1] LOG C-0x5631ef481e80: (nodb)/(nouser)@172.22.0.1:38600 closing because: failed cancel request (age=0s)
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.506 UTC [1] LOG C-0x5631ef481e80: shard1/[email protected]:38606 login attempt: db=shard1 user=postgres tls=no
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.508 UTC [1] LOG S-0x5631ef4891d0: shard1/[email protected]:5432 new connection to server (from 172.22.0.6:33094)

[drakkan]

@jackc

The only reason the code originally read was to wait for the server to close the connection.

That's exactly the reason I want to revert this commit. My current understanding is that due to the usage of PGbouncer, the connection is not fast enough to deliver the instruction and defer cancelConn.Close() closes the connection earlier than the instruction is delivered. And the query keeps running as a result.

Here's the pgbouncer log:

kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:51.238 UTC [1] LOG S-0x5631ef4891d0: shard1/[email protected]:5432 new connection to server (from 172.22.0.6:33088)
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.005 UTC [1] LOG C-0x5631ef481e80: shard1/[email protected]:57294 closing because: client close request (age=345s)
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.005 UTC [1] LOG S-0x5631ef4891d0: shard1/[email protected]:5432 closing because: client disconnect while server was not ready (age=2s)
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.005 UTC [1] LOG C-0x5631ef481e80: (nodb)/(nouser)@172.22.0.1:38600 closing because: failed cancel request (age=0s)
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.506 UTC [1] LOG C-0x5631ef481e80: shard1/[email protected]:38606 login attempt: db=shard1 user=postgres tls=no
kpis-pgbouncer-shard1-1  | 2023-10-09 14:42:54.508 UTC [1] LOG S-0x5631ef4891d0: shard1/[email protected]:5432 new connection to server (from 172.22.0.6:33094)

in this case I think we can just read and ignore the error but let's see what @jackc thinks

I mean something like this

cancelConn.Read(buf)

and always return a nil error.
A comment explaining the reason for this would prevent others from doing my same mistake 😄

[za-arthur]

FYI here is how Postgres libpq sends cancel request:
https://github.com/postgres/postgres/blob/REL_16_0/src/interfaces/libpq/fe-connect.c#L4946-L4960

Looks like it waits for a response from a server and ignores all errors. AFAIK EINTR can be ignored by pgx since go runtime handles it.

[jackc]

LGTM.