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