Re: Considering signal handling in plpython again

On 10/05/18 09:32, Hubert Zhang wrote:
> Hi all,
> 
> I want to support canceling for a plpython query which may be a busy loop.
> 
> I found some discussions on pgsql-hackers 2 years ago. Below is the link.
> 
> https://www.postgresql.org/message-id/CAFYwGJ3+Xg7EcL2nU-MxX6p+O6c895Pm3mYZ-b+9n9DffEh5MQ@mail.gmail.com
> 
> Mario wrote a patch to fix this problem at that time
> *https://github.com/CartoDB/postgres/commit/d587d8a6e4f035cc45e1d84fc46aa7c3ab0344c3
> <https://github.com/CartoDB/postgres/commit/d587d8a6e4f035cc45e1d84fc46aa7c3ab0344c3>*
> <https://github.com/CartoDB/postgres/commit/d587d8a6e4f035cc45e1d84fc46aa7c3ab0344c3>
> 
> The main logic is to register a new signal handler for SIGINT/SIGTERM
> and link the old signal handler in the chain.
> 
> static void PLy_python_interruption_handler()
> {
> PyErr_SetString(PyExc_RuntimeError, "test except");
> return NULL;
> }
> static void
> PLy_handle_interrupt(int sig)
> {
> // custom interruption
> int added = Py_AddPendingCall(PLy_python_interruption_handler, NULL);
> if (coreIntHandler) {
> (*coreIntHandler)(sig);
> }
> }
> 
> Does anyone have some comments on this patch?

PostgreSQL assumes to have control of all the signals. Although I don't 
foresee any changes in this area any time soon, there's no guarantee 
that overriding the SIGINT/SIGTERM will do what you want in the future. 
Also, catching SIGINT/SIGTERM still won't react to recovery conflict 
interrupts.

In that old thread, I think the conclusion was that we should provide a 
hook in the backend for this, rather than override the signal handler 
directly. We could then call the hook whenever InterruptPending is set. 
No-one got around to write a patch to do that, though.

> As for me, I think handler function should call PyErr_SetInterrupt()
> instead of PyErr_SetString(PyExc_RuntimeError, "test except");

Hmm. I tested that, but because the Python's default SIGINT handler is 
not installed, PyErr_SetInterrupt() will actually throw an error:

TypeError: 'NoneType' object is not callable

I came up with the attached patch, which is similar to Mario's, but it 
introduces a new "hook" for this.

One little problem remains: The Py_AddPendingCall() call is made 
unconditionally in the signal handler, even if no Python code is 
currently being executed. The pending call is queued up until the next 
time you run a PL/Python function, which could be long after the 
original statement was canceled. We need some extra checks to only raise 
the Python exception, if the Python interpreter is currently active.

- Heikki