Considering signal handling in plpython again

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Considering signal handling in plpython again

Hubert Zhang
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.


Mario wrote a patch to fix this problem at that time

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?
As for me, I think handler function should call PyErr_SetInterrupt() instead of PyErr_SetString(PyExc_RuntimeError, "test except");

--
Thanks

Hubert Zhang
Reply | Threaded
Open this post in threaded view
|

Re: Considering signal handling in plpython again

Heikki Linnakangas
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@...
>
> 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

plpython-cancel-1.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Considering signal handling in plpython again

Robert Haas
On Thu, May 10, 2018 at 10:50 AM, Heikki Linnakangas <[hidden email]> wrote:
> I came up with the attached patch, which is similar to Mario's, but it
> introduces a new "hook" for this.

I'd rather have the hook be executed whenever ProcessInterrupts() is
called, rather than only in the query-cancel case.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Reply | Threaded
Open this post in threaded view
|

Re: Considering signal handling in plpython again

Hubert Zhang
In reply to this post by Heikki Linnakangas
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 <[hidden email]> 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-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



--
Thanks

Hubert Zhang
Reply | Threaded
Open this post in threaded view
|

Re: Considering signal handling in plpython again

Heikki Linnakangas


On 11 May 2018 10:01:56 EEST, Hubert Zhang <[hidden email]> wrote:
>2. Add a flag in hook function to indicate whether to call
>Py_AddPendingCall.
>This is straightforward.(I prefer it)

Yeah, that's what I had in mind, too. A global bool variable that's set when you enter libpython, and cleared on return. Need to handle nesting, i.e if a PL/python function runs a slow query with SPI, and cancellation happens during that. And the case that the SPI query calls another PL/python function.

- Heikki

Reply | Threaded
Open this post in threaded view
|

Re: Considering signal handling in plpython again

Hubert Zhang
For nested SPI case, one option is to turn off the bool variable when entering the SPI function(PLy_spi_prepare, PLy_spi_execute, PLy_cursor etc.)
and turn on the bool variable again when exiting the SPI function.
If it's OK, we could follow this way to update Heikki's patch.

--Hubert

On Fri, May 11, 2018 at 9:28 PM, Heikki Linnakangas <[hidden email]> wrote:


On 11 May 2018 10:01:56 EEST, Hubert Zhang <[hidden email]> wrote:
>2. Add a flag in hook function to indicate whether to call
>Py_AddPendingCall.
>This is straightforward.(I prefer it)

Yeah, that's what I had in mind, too. A global bool variable that's set when you enter libpython, and cleared on return. Need to handle nesting, i.e if a PL/python function runs a slow query with SPI, and cancellation happens during that. And the case that the SPI query calls another PL/python function.

- Heikki



--
Thanks

Hubert Zhang
Reply | Threaded
Open this post in threaded view
|

Re: Considering signal handling in plpython again

Heikki Linnakangas
On 14/05/18 10:56, Hubert Zhang wrote:
> For nested SPI case, one option is to turn off the bool variable when
> entering the SPI function(PLy_spi_prepare, PLy_spi_execute, PLy_cursor etc.)
> and turn on the bool variable again when exiting the SPI function.

Yeah, that seems reasonable.

- Heikki

Reply | Threaded
Open this post in threaded view
|

Re: Considering signal handling in plpython again

Hubert Zhang
There are remaining two problems

1. Do we need to consider when to delete the extension hook or it's not necessary?
    As the destroy function _PG_fini doesn't work, I cannot find a good place to reset to hook gracefully.
    I tested the drop language plpythonu statement which will not remove the python shared library in the current session,
    So it seems to be safe to leave the cancel_handler_hook not be reset.
    How about other extensions, for example plr. Does the "drop extension" statement will not remove the loaded shared library in the process either?
    
    -- Another idea is to register the hook at the beginning of plpython_call_handler and unregister the hook at the end of plpython_call_handler.

2. Do we need to use explicit hook list(List *cancel_hook_list) instead of implicit cancel_hook(which relies on the extension to link the cancel_hook inside their code 
     e.g. prev_hook = cancel_hook; cancel_hook=my_hook;   void my_hook(){mywork(); (*prev_hook)();} )?
    I didn't find any explicit hook list in PG code base, is that a good practice?

-- Hubert


On Mon, May 14, 2018 at 6:40 PM, Heikki Linnakangas <[hidden email]> wrote:
On 14/05/18 10:56, Hubert Zhang wrote:
For nested SPI case, one option is to turn off the bool variable when
entering the SPI function(PLy_spi_prepare, PLy_spi_execute, PLy_cursor etc.)
and turn on the bool variable again when exiting the SPI function.

Yeah, that seems reasonable.

- Heikki



--
Thanks

Hubert Zhang
Reply | Threaded
Open this post in threaded view
|

Re: Considering signal handling in plpython again

Heikki Linnakangas


On 16 May 2018 16:50:24 EEST, Hubert Zhang <[hidden email]> wrote:
>There are remaining two problems
>
>1. Do we need to consider when to delete the extension hook or it's not
>necessary?

No, PostgreSQL never unloads shared libraries, so that can be ignored.

We used to, and that's what _PG_fini() was for. But we stopped doing that, because it was unsafe. IIRC, hook functions like this was exactly the reason that made it unsafe.

>2. Do we need to use explicit hook list(List *cancel_hook_list) instead
>of
>implicit cancel_hook(which relies on the extension to link the
>cancel_hook
>inside their code
>     e.g. prev_hook = cancel_hook; cancel_hook=my_hook;   void
>my_hook(){mywork(); (*prev_hook)();} )?
>   I didn't find any explicit hook list in PG code base, is that a good
>practice

I didn't understand what you meant with a hook list. But I believe the way I had the hook in the patch was fine.

- Heikki

Previous Thread Next Thread