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

Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> Since PQrequestCancel() is marked as deprecated, I don't think that
> we need to add the feature into it.

Not absolutely necessary, agreed, but it looks like it's pretty
easy to make that happen, so why not?  I'd suggest dropping the
separate implementation and turning PQrequestCancel() into a
thin wrapper around PQgetCancel, PQcancel, PQfreeCancel.

> libpq has already setKeepalivesXXX() functions to do the almost same thing.
> Isn't it better to modify and reuse them instead of adding the almost
> same code, to simplify the code?

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.

I see that setsockopt(2) is specified to be async signal safe by
POSIX, so at least in principle it is okay to add those calls to
PQcancel.  But you've got to be really careful what else you do in
there.  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'm not sure that WSAIoctl
is safe to use in a signal handler, so on the whole I think
I'd drop the Windows-specific chunk altogether.  But in any case,
I'm very strongly against calling out to other libpq code from here,
because then the signal-safety restrictions start applying to that
other code too, and that's a recipe for trouble in future.

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

			regards, tom lane