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

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