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