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

From: Jelte Fennema <Jelte(dot)Fennema(at)microsoft(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)anarazel(dot)de>
Cc: Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, Zhihong Yu <zyu(at)yugabyte(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [EXTERNAL] Re: PQcancel does not use tcp_user_timeout, connect_timeout and keepalive settings
Date: 2022-01-06 15:58:28
Message-ID: AM5PR83MB01782D9A7D607B0419B3CB47F74C9@AM5PR83MB0178.EURPRD83.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-Refactor-to-remove-internal_cancel-helper.patch application/octet-stream 4.9 KB
0002-Use-timeout-socket-options-for-cancel-connections.patch application/octet-stream 6.5 KB
0003-Honor-connect_timeout-when-connecting-with-PQcancel.patch application/octet-stream 6.4 KB
0001-Doc-explain-that-connection-options-don-t-apply-to-c.patch application/octet-stream 4.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Joel Jacobson 2022-01-06 15:58:57 Re: pl/pgsql feature request: shorthand for argument and local variable references
Previous Message Tom Lane 2022-01-06 15:24:09 Re: sqlsmith: ERROR: XX000: bogus varno: 2