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
- 0001-Refactor-to-remove-internal_cancel-helper.patch (application/octet-stream) patch 0001
- 0002-Use-timeout-socket-options-for-cancel-connections.patch (application/octet-stream) patch 0002
- 0003-Honor-connect_timeout-when-connecting-with-PQcancel.patch (application/octet-stream) patch 0003
- 0001-Doc-explain-that-connection-options-don-t-apply-to-c.patch (application/octet-stream) patch 0001
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
-
Make PQcancel use the PGconn's tcp_user_timeout and keepalives settings.
- 5987feb70b5b 15.0 landed
-
Avoid calling gettext() in signal handlers.
- d18ec312f9f3 13.6 landed
- 92e6c1c9be15 11.15 landed
- 6d1a854c157e 12.10 landed
- 62bfa554b285 10.20 landed
- 4e8726566e4d 14.2 landed
- 2131c049d338 15.0 landed
-
Avoid calling strerror[_r] in PQcancel().
- 9d66c43eb4c8 10.20 landed
- f3f467b8f69b 15.0 landed
- f27af7b880de 13.6 landed
- 8b107467c632 11.15 landed
- 38f099ef935b 12.10 landed
- 050949877004 14.2 landed