public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Jelte Fennema <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: Zhihong Yu <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: [EXTERNAL] Re: PQcancel does not use tcp_user_timeout, connect_timeout and keepalive settings
Date: Wed, 5 Jan 2022 10:42:25 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <AM5PR83MB0178DCFFC1BE77C4C390DA89F7439@AM5PR83MB0178.EURPRD83.prod.outlook.com>
References: <AM5PR83MB017870DE81FC84D5E21E9D1EF7AA9@AM5PR83MB0178.EURPRD83.prod.outlook.com>
<CALNJ-vSbOyVm9nk3y1UwA3e+M8atwJtTg_3ugRfC5W+TyAbMZQ@mail.gmail.com>
<AM5PR83MB01781C8E94AEC8073B646872F7B09@AM5PR83MB0178.EURPRD83.prod.outlook.com>
<AM5PR83MB0178586E6B079DF8E7F0D855F7B09@AM5PR83MB0178.EURPRD83.prod.outlook.com>
<[email protected]>
<[email protected]>
<HE1PR83MB0186173FC957C2554D0A31B6F7939@HE1PR83MB0186.EURPRD83.prod.outlook.com>
<AM5PR83MB0178DCFFC1BE77C4C390DA89F7439@AM5PR83MB0178.EURPRD83.prod.outlook.com>
Hi,
On 2021-12-28 15:49:00 +0000, Jelte Fennema wrote:
> The first patch is a cleaned up version of my previous patch. I think I addressed
> all feedback on the previous version in that patch (e.g. removed windows code,
> fixed formatting).
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?
> The second patch is a new one, it implements honouring of the connect_timeout
> connection option in PQcancel. This patch requires the first patch to also be applied,
> but since it seemed fairly separate and the code is not trivial I didn't want the first
> patch to be blocked on this.
>
> Finally, I would love it if once these fixes are merged the would also be backpatched to
> previous versions of libpq. Does that seem possible? As far as I can tell it would be fine,
> since it doesn't really change any of the public APIs. The only change is that the pg_cancel
> struct now has a few additional fields. But since that struct is defined in libpq-int.h, so that
> struct should not be used by users of libpq directly, right?.
I'm not really convinced this is a good patch to backpatch. There does seem to
be some potential for subtle breakage - code in signal handlers is notoriously
finnicky, it's a rarely exercised code path, etc. It's also not fixing
something that previously worked.
> + * NOTE: These socket options are currently not set for Windows. The
> + * reason is that signal safety in this function is very important, and it
> + * was not clear to if the functions required to set the socket options on
> + * Windows were signal-safe.
> + */
> +#ifndef WIN32
> + if (!IS_AF_UNIX(cancel->raddr.addr.ss_family))
> + {
> +#ifdef TCP_USER_TIMEOUT
> + if (cancel->pgtcp_user_timeout >= 0)
> + {
> + if (setsockopt(tmpsock, IPPROTO_TCP, TCP_USER_TIMEOUT,
> + (char *) &cancel->pgtcp_user_timeout,
> + sizeof(cancel->pgtcp_user_timeout)) < 0)
> + {
> + strlcpy(errbuf, "PQcancel() -- setsockopt(TCP_USER_TIMEOUT) failed: ", errbufsize);
> + goto cancel_errReturn;
> + }
> + }
> +#endif
> +
> + if (cancel->keepalives != 0)
> + {
> + int on = 1;
> +
> + if (setsockopt(tmpsock,
> + SOL_SOCKET, SO_KEEPALIVE,
> + (char *) &on, sizeof(on)) < 0)
> + {
> + strlcpy(errbuf, "PQcancel() -- setsockopt(SO_KEEPALIVE) failed: ", errbufsize);
> + goto cancel_errReturn;
> + }
> + }
This is very repetitive - how about introducing a helper function for this?
> @@ -4467,8 +4601,8 @@ retry3:
>
> crp.packetlen = pg_hton32((uint32) sizeof(crp));
> crp.cp.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
> - crp.cp.backendPID = pg_hton32(be_pid);
> - crp.cp.cancelAuthCode = pg_hton32(be_key);
> + crp.cp.backendPID = pg_hton32(cancel->be_pid);
> + crp.cp.cancelAuthCode = pg_hton32(cancel->be_key);
Others might differ, but I'd separate changing the type passed to
internal_cancel() into its own commit.
Greetings,
Andres Freund
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [EXTERNAL] Re: PQcancel does not use tcp_user_timeout, connect_timeout and keepalive settings
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox