Обсуждение: 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

On Thu, May 10, 2018 at 10:50 AM, Heikki Linnakangas <hlinnaka@iki.fi> 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

On 11 May 2018 10:01:56 EEST, Hubert Zhang <hzhang@pivotal.io> 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

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

On 16 May 2018 16:50:24 EEST, Hubert Zhang <hzhang@pivotal.io> 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
likethis 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

Hi Hubert,

Are you working on this, or should I pick this up? Would be nice to get 
this done as soon as v12 development begins.

- Heikki