From 1af0aec5654b712cf3a5d20b0d1ee4303abcce02 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 21 Apr 2017 18:54:51 +0200 Subject: [PATCH 1/8] Fix problem with INT signals outside of plpython execution INT signals received when python code is not being executed affected code executed later. --- debian/patches/92-plpython-interrupt.patch | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/debian/patches/92-plpython-interrupt.patch b/debian/patches/92-plpython-interrupt.patch index 255e22b..5029e39 100644 --- a/debian/patches/92-plpython-interrupt.patch +++ b/debian/patches/92-plpython-interrupt.patch @@ -57,7 +57,11 @@ index 5dd86c6..b2632cd 100644 +PLy_handle_interrupt(int sig) +{ + // custom interruption -+ Py_AddPendingCall(PLy_python_interruption_handler, NULL); ++ if (PLy_execution_contexts != NULL) { ++ // Insert python interruption only if a python function is being executed ++ // otherwise, future python execution would be affected ++ Py_AddPendingCall(PLy_python_interruption_handler, NULL); ++ } + + if (coreIntHandler) { + (*coreIntHandler)(sig); From 85d9a312b39e9d0d12358aecf17060a1a0cf2643 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 25 Apr 2017 11:57:57 +0200 Subject: [PATCH 2/8] More strict plpython clean up The previous signal handler should always be restore upon module clean up. Note that a null pointer may be a valid handler specifier (SIG_DFL actually). We rely on _PG_fini calls being always paired with _PG_init. Note also that as of PostgreSQL 9.6 _PG_init isn't actually called ever (modules are never unloaded). --- debian/patches/92-plpython-interrupt.patch | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/debian/patches/92-plpython-interrupt.patch b/debian/patches/92-plpython-interrupt.patch index 5029e39..f84a618 100644 --- a/debian/patches/92-plpython-interrupt.patch +++ b/debian/patches/92-plpython-interrupt.patch @@ -41,9 +41,7 @@ index 5dd86c6..b2632cd 100644 +void +_PG_fini(void) +{ -+ if (coreIntHandler) { -+ pqsignal(SIGINT, coreIntHandler); -+ } ++ pqsignal(SIGINT, coreIntHandler); +} + +static int From 3644014d29826fead496ad896291ee591c4aa8e7 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 3 May 2017 10:11:07 +0200 Subject: [PATCH 3/8] Avoid potential problems with SIG_DFL, SIG_IGN --- debian/patches/92-plpython-interrupt.patch | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/debian/patches/92-plpython-interrupt.patch b/debian/patches/92-plpython-interrupt.patch index f84a618..3405289 100644 --- a/debian/patches/92-plpython-interrupt.patch +++ b/debian/patches/92-plpython-interrupt.patch @@ -61,7 +61,8 @@ index 5dd86c6..b2632cd 100644 + Py_AddPendingCall(PLy_python_interruption_handler, NULL); + } + -+ if (coreIntHandler) { ++ if (coreIntHandler != SIG_DFL && coreIntHandler != SIG_IGN) { ++ // Note that we're handling SIG_DFL (incorrectly) as SIG_IGN + (*coreIntHandler)(sig); + } +} From d39984ddafec622772c2238e7f955c7e35ab72ed Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 3 May 2017 16:18:52 +0200 Subject: [PATCH 4/8] Fix pending call return value. Returning 0 from the pending call function has the effect of not raising errors immediately. We must return -1 to make sure any errors are trapped in the current context. --- debian/patches/92-plpython-interrupt.patch | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debian/patches/92-plpython-interrupt.patch b/debian/patches/92-plpython-interrupt.patch index 3405289..d492ff5 100644 --- a/debian/patches/92-plpython-interrupt.patch +++ b/debian/patches/92-plpython-interrupt.patch @@ -48,7 +48,7 @@ index 5dd86c6..b2632cd 100644 +PLy_python_interruption_handler() +{ + PyErr_SetString(PyExc_RuntimeError, "Execution of function interrupted by signal"); -+ return 0; ++ return -1; +} + +static void From ac37ab134d1d9d2f97037d9fa2259372e1f8d683 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 3 May 2017 17:08:08 +0200 Subject: [PATCH 5/8] Prevent out-of-context Python interruptions Make sure Python exceptions occur in the context where the SIGINT occured --- debian/patches/92-plpython-interrupt.patch | 64 +++++++++++++++++----- 1 file changed, 49 insertions(+), 15 deletions(-) diff --git a/debian/patches/92-plpython-interrupt.patch b/debian/patches/92-plpython-interrupt.patch index d492ff5..f1fb415 100644 --- a/debian/patches/92-plpython-interrupt.patch +++ b/debian/patches/92-plpython-interrupt.patch @@ -14,25 +14,52 @@ index 5dd86c6..b2632cd 100644 @@ -74,6 +74,10 @@ PyObject *PLy_interp_globals = NULL; /* this doesn't need to be global; use PLy_current_execution_context() */ static PLyExecutionContext *PLy_execution_contexts = NULL; - + +/* postgres backend handler for interruption */ +static pqsigfunc coreIntHandler = 0; +static void PLy_handle_interrupt(int sig); -+ - + void _PG_init(void) -@@ -81,6 +85,9 @@ _PG_init(void) +@@ -81,6 +84,9 @@ _PG_init(void) int **bitmask_ptr; const int **version_ptr; - -+ // Catch and process signals -+ coreIntHandler = pqsignal(SIGINT, PLy_handle_interrupt); + ++ // Catch and process signals ++ coreIntHandler = pqsignal(SIGINT, PLy_handle_interrupt); + /* * Set up a shared bitmask variable telling which Python version(s) are * loaded into this process's address space. If there's more than one, we -@@ -454,3 +461,31 @@ PLy_pop_execution_context(void) +@@ -425,6 +431,9 @@ PLy_current_execution_context(void) + return PLy_execution_contexts; + } + ++static int PLy_execution_context_level = 0; ++static int PLy_pending_interrupt = 0; ++ + static PLyExecutionContext * + PLy_push_execution_context(void) + { +@@ -438,6 +447,7 @@ PLy_push_execution_context(void) + ALLOCSET_DEFAULT_MAXSIZE); + context->next = PLy_execution_contexts; + PLy_execution_contexts = context; ++ ++PLy_execution_context_level; + return context; + } + +@@ -449,8 +459,48 @@ PLy_pop_execution_context(void) + if (context == NULL) + elog(ERROR, "no Python function is currently executing"); + ++ if (--PLy_execution_context_level == 0) { ++ // Clear pending interrupts when top level context exits ++ PLy_pending_interrupt = 0; ++ } ++ + PLy_execution_contexts = context->next; + MemoryContextDelete(context->scratch_ctx); PLy_free(context); } @@ -41,12 +68,18 @@ index 5dd86c6..b2632cd 100644 +void +_PG_fini(void) +{ -+ pqsignal(SIGINT, coreIntHandler); ++ // Restore previous SIGINT handler ++ pqsignal(SIGINT, coreIntHandler); +} + +static int +PLy_python_interruption_handler() +{ ++ if (!PLy_pending_interrupt) { ++ return 0; ++ } ++ ++ PLy_pending_interrupt = 0; + PyErr_SetString(PyExc_RuntimeError, "Execution of function interrupted by signal"); + return -1; +} @@ -54,16 +87,17 @@ index 5dd86c6..b2632cd 100644 +static void +PLy_handle_interrupt(int sig) +{ -+ // custom interruption -+ if (PLy_execution_contexts != NULL) { ++ if (PLy_execution_context_level > 0) { + // Insert python interruption only if a python function is being executed + // otherwise, future python execution would be affected -+ Py_AddPendingCall(PLy_python_interruption_handler, NULL); ++ if (!PLy_pending_interrupt) { ++ PLy_pending_interrupt = 1; ++ Py_AddPendingCall(PLy_python_interruption_handler, PLy_execution_contexts); ++ } + } -+ ++ // Fallback to prior handlers + if (coreIntHandler != SIG_DFL && coreIntHandler != SIG_IGN) { -+ // Note that we're handling SIG_DFL (incorrectly) as SIG_IGN ++ Note that we're handling SIG_DFL (incorrectly) as SIG_IGN + (*coreIntHandler)(sig); + } +} -+ From 5562521671f4b9c9cdb1f804e0c724cb986ea85c Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Fri, 5 May 2017 12:09:36 +0200 Subject: [PATCH 6/8] Simplify plpython context tracking Remove unnecessary remnants from previous tests: * No need to keep track of plpython context nesting level * No need to pass arguments to the python pending call --- debian/patches/92-plpython-interrupt.patch | 35 +++++++--------------- 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/debian/patches/92-plpython-interrupt.patch b/debian/patches/92-plpython-interrupt.patch index f1fb415..a9a5380 100644 --- a/debian/patches/92-plpython-interrupt.patch +++ b/debian/patches/92-plpython-interrupt.patch @@ -35,31 +35,20 @@ index 5dd86c6..b2632cd 100644 return PLy_execution_contexts; } -+static int PLy_execution_context_level = 0; ++/* Indicate tha a python interruption is pending */ +static int PLy_pending_interrupt = 0; + static PLyExecutionContext * PLy_push_execution_context(void) { -@@ -438,6 +447,7 @@ PLy_push_execution_context(void) - ALLOCSET_DEFAULT_MAXSIZE); - context->next = PLy_execution_contexts; - PLy_execution_contexts = context; -+ ++PLy_execution_context_level; - return context; - } - -@@ -449,8 +459,48 @@ PLy_pop_execution_context(void) - if (context == NULL) - elog(ERROR, "no Python function is currently executing"); +@@ -451,6 +460,47 @@ PLy_pop_execution_context(void) + PLy_execution_contexts = context->next; -+ if (--PLy_execution_context_level == 0) { ++ if (PLy_execution_contexts == NULL) { + // Clear pending interrupts when top level context exits + PLy_pending_interrupt = 0; + } + - PLy_execution_contexts = context->next; - MemoryContextDelete(context->scratch_ctx); PLy_free(context); } @@ -87,17 +76,15 @@ index 5dd86c6..b2632cd 100644 +static void +PLy_handle_interrupt(int sig) +{ -+ if (PLy_execution_context_level > 0) { -+ // Insert python interruption only if a python function is being executed -+ // otherwise, future python execution would be affected -+ if (!PLy_pending_interrupt) { -+ PLy_pending_interrupt = 1; -+ Py_AddPendingCall(PLy_python_interruption_handler, PLy_execution_contexts); -+ } ++ if (PLy_execution_contexts != NULL && !PLy_pending_interrupt) { ++ PLy_pending_interrupt = 1; ++ Py_AddPendingCall(PLy_python_interruption_handler, NULL); + } -+ // Fallback to prior handlers ++ // Fallback to execute prior handlers + if (coreIntHandler != SIG_DFL && coreIntHandler != SIG_IGN) { -+ Note that we're handling SIG_DFL (incorrectly) as SIG_IGN ++ // There's a catch here: if the prior handler was SIG_DFL we have no easy way ++ // of invoking it here; ++ // As that's an unlikely situation we'll just treat SIG_DFL as SIG_IGN. + (*coreIntHandler)(sig); + } +} From 34d59936538cd766135fc75ff8ef27073f4a6a71 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Tue, 23 May 2017 18:24:55 +0200 Subject: [PATCH 7/8] New patch based on jgoizueta's work See https://github.com/CartoDB/postgres/pull/5 and more specifically the commit b319cee --- debian/patches/92-plpython-interrupt.patch | 35 +++++++++++----------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/debian/patches/92-plpython-interrupt.patch b/debian/patches/92-plpython-interrupt.patch index a9a5380..b07cf50 100644 --- a/debian/patches/92-plpython-interrupt.patch +++ b/debian/patches/92-plpython-interrupt.patch @@ -1,31 +1,30 @@ -From c7f7c818d90b8f94103b218732ba749db6812ea9 Mon Sep 17 00:00:00 2001 -From: Mario de Frutos -Date: Fri, 14 Oct 2016 18:44:31 +0200 -Subject: [PATCH] Handler to manage interrupts for python code inside PLPython +commit b319ceef00d3a9cfd5b7a4bd0bc9a892ca18149b +Author: Javier Goizueta +Date: Fri May 5 12:27:15 2017 +0200 ---- - src/pl/plpython/plpy_main.c | 35 +++++++++++++++++++++++++++++++++++ - 1 file changed, 35 insertions(+) + Make execution of plpython interruptible + + The plpy module hooks into the signal handling mechanism to insert python exceptions when a SIGINT occurs. diff --git a/src/pl/plpython/plpy_main.c b/src/pl/plpython/plpy_main.c -index 5dd86c6..b2632cd 100644 +index 5dd86c6..9f53453 100644 --- a/src/pl/plpython/plpy_main.c +++ b/src/pl/plpython/plpy_main.c -@@ -74,6 +74,10 @@ PyObject *PLy_interp_globals = NULL; +@@ -74,6 +74,9 @@ PyObject *PLy_interp_globals = NULL; /* this doesn't need to be global; use PLy_current_execution_context() */ static PLyExecutionContext *PLy_execution_contexts = NULL; - + +/* postgres backend handler for interruption */ +static pqsigfunc coreIntHandler = 0; +static void PLy_handle_interrupt(int sig); - + void _PG_init(void) @@ -81,6 +84,9 @@ _PG_init(void) int **bitmask_ptr; const int **version_ptr; - -+ // Catch and process signals + ++ /* Catch and process SIGINT signals */ + coreIntHandler = pqsignal(SIGINT, PLy_handle_interrupt); + /* @@ -34,16 +33,17 @@ index 5dd86c6..b2632cd 100644 @@ -425,6 +431,9 @@ PLy_current_execution_context(void) return PLy_execution_contexts; } - + +/* Indicate tha a python interruption is pending */ +static int PLy_pending_interrupt = 0; + static PLyExecutionContext * PLy_push_execution_context(void) { -@@ -451,6 +460,47 @@ PLy_pop_execution_context(void) - PLy_execution_contexts = context->next; - +@@ -451,6 +460,46 @@ PLy_pop_execution_context(void) + + PLy_execution_contexts = context->next; + + if (PLy_execution_contexts == NULL) { + // Clear pending interrupts when top level context exits + PLy_pending_interrupt = 0; @@ -53,7 +53,6 @@ index 5dd86c6..b2632cd 100644 PLy_free(context); } + -+void _PG_fini(void); +void +_PG_fini(void) +{ From 12b89e033b494b62ccd4bb7f774d0032fe71f910 Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Tue, 23 May 2017 18:51:13 +0200 Subject: [PATCH 8/8] Release 9.5.2-3cdb3 --- debian/changelog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/debian/changelog b/debian/changelog index f45094e..d6e15ff 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,9 @@ +postgresql-9.5 (9.5.2-3cdb3) precise; urgency=medium + + * Improved patch for interruptible PLPython functions + + -- Rafa de la Torre Tue, 23 May 2017 18:48:02 +0200 + postgresql-9.5 (9.5.2-3cdb2) precise; urgency=low * Release 9.5.2-3cdb2