Re: Converting pqsignal to void return

Hi,

On 2025-01-22 07:57:52 -0800, Paul Ramsey wrote:
> I just ran across this change when a bunch of CI’s I run barfed.
>
> https://github.com/postgres/postgres/commit/d4a43b283751b23d32bbfa1ecc2cad2d16e3dde9
>
> The problem we are having in extension land is that we often run functions
> in external libraries that take a long time to return, and we would like to
> ensure that PgSQL users can cancel their queries, even when control has
> passed into those functions.
>
> The way we have done it, historically, has been to take the return value of
> pqsignal(SIGINT, extension_signint_handler) and remember it, and then,
> inside extension_signint_handler, call the pgsql handler once we have done
> our own business.

I think that's a really bad idea. For one, postgres might use signals in
different process types for different things. We are also working on not using
plain signals in a bunch of places.

It's also really hard to write correct signal handlers. E.g.:

> https://github.com/pramsey/pgsql-http/blob/master/http.c#L345

That signal handler is profoundly unsafe:

http_interrupt_handler(int sig)
{
/* Handle the signal here */
elog(DEBUG2, "http_interrupt_handler: sig=%d", sig);
http_interrupt_requested = sig;
pgsql_interrupt_handler(sig);
return;
}

You're definitely not allowed to elog in a signal handler unless you take
*extraordinary* precautions (basically converting the normal execution path to
only perform async-signal-safe work).

This signal handler can e.g. corrupt the memory context used by elog.c,
deadlock in the libc memory allocator, break the FE/BE protocol if any client
ever set client_min_messages=debug2, jump out of a signal handler in case of
an allocation failure and many other things.

> It is possible we have been Doing It Wrong all this time, and would love some pointers on the right way to do this.

All your interrupt handler is doing "for you" is setting
http_interrupt_requested, right? Why do you need that variable? Seems you
could just check postgres' QueryCancelPending? And perhaps ProcDiePending, if
you want to handle termination too.

Greetings,

Andres Freund