Re: Considering signal handling in plpython again - Mailing list pgsql-hackers
| From | Hubert Zhang |
|---|---|
| Subject | Re: Considering signal handling in plpython again |
| Date | |
| Msg-id | CAB0yre=ujAFTVMOawpToC9MBcoocDorcChrkU+y4NHKbZy+MFw@mail.gmail.com Whole thread Raw |
| In response to | Re: Considering signal handling in plpython again (Heikki Linnakangas <hlinnaka@iki.fi>) |
| Responses |
Re: Considering signal handling in plpython again
(Heikki Linnakangas <hlinnaka@iki.fi>)
|
| List | pgsql-hackers |
Thanks Heikki and Robert for your comments.
I reviewed Heikki's patch and let's enhance it.
As Heikki mentioned, there is a problem when no Python code is being executed. I tested it in the following case
"select pysleep();" and then type ctrl-c, query cancelled successfully.(Patch works:))
"select * from a, b, c, d;" and then type ctrl-c, query cancelled successfully.
"select pysleep();" It will terminate immediately(without type ctrl-c) due to the Py_AddPendingCall is registered on the last query.(The problem Heikki said)
To fix this problem, we need to let query like "select * from a, b, c, d;" doesn't call Py_AddPendingCall.
There are at least 3 ways.
1. Add a flag in hook function to indicate whether to call the hook function.
In current patch the hook function will do two things: a. call Py_AddPendingCall; b. call the previous hook(prev_cancel_pending_hook).
So this method will introduce side effect on miss the previous hook. To enhance it, we need to change the hook in postgres.c to hook array.
And let hook function only do one thing.
2. Add a flag in hook function to indicate whether to call Py_AddPendingCall. This is straightforward.(I prefer it)
3. Delete the hook after it's been used. This is related to the lifecycle of signal hooks. In current patch the handler hook is registered inside PLy_initialize() which will be called for every plpython_call_handler(). I prefer to just register hook once and in _PG_init() for each extension. If follow this way, delete hook is not needed.
Any comments?
On Thu, May 10, 2018 at 10:50 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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-MxX 6p+O6c895Pm3mYZ-b+9n9DffEh5MQ@ mail.gmail.com
Mario wrote a patch to fix this problem at that time
*https://github.com/CartoDB/postgres/commit/d587d8a6e4f035cc 45e1d84fc46aa7c3ab0344c3
<https://github.com/CartoDB/postgres/commit/d587d8a6e4f035cc 45e1d84fc46aa7c3ab0344c3>*
<https://github.com/CartoDB/postgres/commit/d587d8a6e4f035cc 45e1d84fc46aa7c3ab0344c3>
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
Thanks
Hubert Zhang
pgsql-hackers by date: