Re: [EXTERNAL] Re: PQcancel does not use tcp_user_timeout, connect_timeout and keepalive settings

Jelte Fennema-Nio <jelte.fennema@microsoft.com>

From: Jelte Fennema <Jelte.Fennema@microsoft.com>
To: Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>, Zhihong Yu <zyu@yugabyte.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2022-01-06T15:58:28Z
Lists: pgsql-hackers

Attachments

Attached are 3 patches that address the feedback from Andres about code duplication 
and splitting up commits. I completely removed internal_cancel now, since it only had 
one caller at this point.

> IMO, this is a new feature not a bug fix

IMO this is definitely a bugfix. Nowhere in the libpq docs it stated that the connection 
options in question do not apply to connections that are opened for cancellations. So 
as a user I definitely expect that any connections that libpq opens would use these options.
Which is why I definitely consider it a bug that they are currently not honoured for cancel 
requests. 

However, even though I think it's a bugfix, I can understand the being hesitant to 
backport this. IMHO in that case at least the docs should be updated to explain this 
discrepancy. I attached a patch to do so against the docs on the REL_14_STABLE branch.

> To me it seems a bit problematic to introduce a divergence between windows /
> everything else here. Isn't that just going to lead to other complaints just
> like this thread, where somebody discovered the hard way that there's platform
> dependent behaviour here?

Of course, fixing this also for windows would be much better. There's two problems:
1. I cannot find any clear documentation on which functions are signal safe in Windows 
   and which are not. The only reference I can find is this: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=msvc-170 
   However, this says that you should not use any function that generates a system call.  
   PQcancel is obviously already violating that when calling "connect", so this is not very helpful.
2. My Windows C experience is non existent, so I don't think I would be the right person to write this code.

IMO blocking this bugfix, because it does not fix it for Windows, would be an example of perfect 
becoming the enemy of good. One thing I could do is add a note to the docs that these options 
are not supported on Windows for cancellation requests (similar to my proposed doc change 
for PG14 and below).

Commits

  1. Make PQcancel use the PGconn's tcp_user_timeout and keepalives settings.

  2. Avoid calling gettext() in signal handlers.

  3. Avoid calling strerror[_r] in PQcancel().