Re: [EXTERNAL] Re: PQcancel does not use tcp_user_timeout, connect_timeout and keepalive settings - Mailing list pgsql-hackers

From Jelte Fennema
Subject Re: [EXTERNAL] Re: PQcancel does not use tcp_user_timeout, connect_timeout and keepalive settings
Date
Msg-id HE1PR83MB0186173FC957C2554D0A31B6F7939@HE1PR83MB0186.EURPRD83.prod.outlook.com
Whole thread Raw
In response to Re: [EXTERNAL] Re: PQcancel does not use tcp_user_timeout, connect_timeout and keepalive settings  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: [EXTERNAL] Re: PQcancel does not use tcp_user_timeout, connect_timeout and keepalive settings
List pgsql-hackers
> I'd suggest dropping the separate implementation and turning 
> PQrequestCancel() into a thin wrapper around PQgetCancel, 
> PQcancel, PQfreeCancel.

Fine by me. I didn't want to change behavior of a deprecated 
function.

> I find this patch fairly scary, because it's apparently been coded
> with little regard for the expectation that PQcancel can be called
> within a signal handler.
> You can NOT use appendPQExpBuffer.  You can NOT access the
> PGconn (I don't think the Windows part of this even compiles ...
> nope, it doesn't, per the cfbot).

I guess I was tired or at least inattentive when I added the windows part at the end
of my coding session. It really was the Linux part that I cared about. For that part 
I definitely took care to make the code signal safe. Which is also why I did not call out to 
any of the existing functions, like setKeepalivesXXX(). I don't think I'm the right person
to write the windows code for this (I have zero C windows experience). So, if it's not 
required for this patch to be accepted I'll happily remove it.

> The patch could use a little attention to conforming to PG coding
> conventions (comment style, brace style, C99 declarations are all
> wrong --- pgindent would fix much of that, but maybe not in a way
> you like).  

Sure, I'll run pgindent for my next version of the patch.

> The lack of comments about why it's doing what it's doing
> needs to be rectified, too.  Why are these specific options important
> and not any others?

I'll make sure to add comments before the final version of this patch. This 
patch was more meant as a draft to gauge if this was even the correct way of fixing 
this problem. 

To be honest I think it would make sense to add a new PQcancel function that is not 
required to be signal safe and reuses regular connection setup code. This would make sure
options like this are supported automatically in the future. Another advantage is that it would 
allow for sending cancel messages in a non-blocking way. So, you would be able to easily 
send multiple cancels in a concurrent way. It looks to me like PQcancel is mostly designed 
the way it is to keep it easy for psql to send cancelations. I think many other uses of PQcancel
don't require it to be signal safe at all (at least for Citus its usage signal safety is not required).

pgsql-hackers by date:

Previous
From: Shay Rojansky
Date:
Subject: Re: Should AT TIME ZONE be volatile?
Next
From: Tom Lane
Date:
Subject: Re: drop tablespace failed when location contains .. on win32