Make execution of plpython interruptible #5
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The plpy module hooks into the signal handling mechanism to insert python exceptions when a SIGINT occurs.
ethervoid
reviewed
May 16, 2017
| @@ -74,13 +74,19 @@ 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 */ | |||
There was a problem hiding this comment.
You left some + symbols from the diff that makes this file fail at compilation time
ethervoid
reviewed
May 17, 2017
| Py_AddPendingCall(PLy_python_interruption_handler, NULL); | ||
| } | ||
| // Fallback to execute prior handlers | ||
| if (coreIntHandler != SIG_DFL && coreIntHandler != SIG_IGN) { |
There was a problem hiding this comment.
SIG_DFL act as a process kill so better to have it out of here :)
ethervoid
reviewed
May 17, 2017
| static int | ||
| PLy_python_interruption_handler() | ||
| { | ||
| if (!PLy_pending_interrupt) { |
There was a problem hiding this comment.
Is this possible? or we're making defensive programming just in case?
There was a problem hiding this comment.
Yes, it will happen when the top PlPython context exits and there are unhandled Python interruption; since we can't remove the pending call we set this flag to 0 to make it have no effect.
ethervoid
approved these changes
May 17, 2017
|
I'm going to need some acceptance steps. I know you worked on some acceptance tests with multiple sessions and pgbouncer :) |
|
Continues on #7 |
rafatower
pushed a commit
to CartoDB/postgresql-debian
that referenced
this pull request
May 23, 2017
See CartoDB/postgres#5 and more specifically the commit b319cee
Algunenano
pushed a commit
that referenced
this pull request
Nov 30, 2018
refresh_by_match_merge() has some issues in the way it builds a SQL query to construct the "diff" table: 1. It doesn't require the selected unique index(es) to be indimmediate. 2. It doesn't pay attention to the particular equality semantics enforced by a given index, but just assumes that they must be those of the column datatype's default btree opclass. 3. It doesn't check that the indexes are btrees. 4. It's insufficiently careful to ensure that the parser will pick the intended operator when parsing the query. (This would have been a security bug before CVE-2018-1058.) 5. It's not careful about indexes on system columns. The way to fix #4 is to make use of the existing code in ri_triggers.c for generating an arbitrary binary operator clause. I chose to move that to ruleutils.c, since that seems a more reasonable place to be exporting such functionality from than ri_triggers.c. While #1, #3, and #5 are just latent given existing feature restrictions, and #2 doesn't arise in the core system for lack of alternate opclasses with different equality behaviors, #4 seems like an issue worth back-patching. That's the bulk of the change anyway, so just back-patch the whole thing to 9.4 where this code was introduced. Discussion: https://postgr.es/m/13836.1521413227@sss.pgh.pa.us
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a postgres process receives a
SIGINT, the default behaviour (handled by the functionStatementCancelHandle) it to set up a couple of flags to indicate the occurrence of the interrupt and hopefully cancel ongoing queries.Since execution of PLPython functions does not check those flags, the execution of long-running code may go uninterrupted.
This was discussed here.
This patch makes execution of PLPython interruptible by hooking into the PostgreSQL signal-handling mechanism with
pqsignal(like for example PostGIS also does) and inserting a pending call into the Python VM to generate a Python exception.Execution could still go uninterrupted if for example long-running external code is called.
Acknowledgements
It was @ethervoid who initially investigated this problem and designed this solution.
Notes on the implementation
Care is taken to avoid inserting or executing python interruptions outside of the proper PLPython contexts. (plpython contexts can be nested as plpython functions are executed through SPI).
Thus we prevent interruptions being effective in later, unrelated queries or commands.
Note also that the python pending call handler must return -1 when an interruption is generated;
failing to do so would cause the interruption to be queued for later execution at an indeterminate time.