Re: Considering signal handling in plpython again

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(at)iki(dot)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(at)mail(dot)gmail(dot)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