Get rid of the global variable holding the error state

Global error handling led to confusion and was hard to manage.  With
this change, errors from PostgreSQL are immediately reported to Python
as exceptions.  This requires setting a Python exception after
reporting the caught PostgreSQL error as a warning, because PLy_elog
destroys the Python exception state.

Ideally, all places where PostgreSQL errors need to be reported back
to Python should be wrapped in subtransactions, to make going back to
Python from a longjmp safe.  This will be handled in a separate patch.

Jan Urbański

diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index 1f24c13..87225f2 100644 (file)
--- a/src/pl/plpython/expected/plpython_error.out
+++ b/src/pl/plpython/expected/plpython_error.out
@@ -10,7 +10,7 @@ CREATE FUNCTION sql_syntax_error() RETURNS text
 SELECT sql_syntax_error();
 WARNING:  PL/Python: plpy.SPIError: unrecognized error in PLy_spi_execute_query
 CONTEXT:  PL/Python function "sql_syntax_error"
-ERROR:  syntax error at or near "syntax"
+ERROR:  PL/Python: plpy.SPIError: syntax error at or near "syntax"
 LINE 1: syntax error
         ^
 QUERY:  syntax error
@@ -34,7 +34,7 @@ return rv[0]'
 SELECT exception_index_invalid_nested();
 WARNING:  PL/Python: plpy.SPIError: unrecognized error in PLy_spi_execute_query
 CONTEXT:  PL/Python function "exception_index_invalid_nested"
-ERROR:  function test5(unknown) does not exist
+ERROR:  PL/Python: plpy.SPIError: function test5(unknown) does not exist
 LINE 1: SELECT test5('foo')
                ^
 HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
@@ -56,7 +56,7 @@ return None
 SELECT invalid_type_uncaught('rick');
 WARNING:  PL/Python: plpy.SPIError: unrecognized error in PLy_spi_prepare
 CONTEXT:  PL/Python function "invalid_type_uncaught"
-ERROR:  type "test" does not exist
+ERROR:  PL/Python: plpy.SPIError: type "test" does not exist
 CONTEXT:  PL/Python function "invalid_type_uncaught"
 /* for what it's worth catch the exception generated by
  * the typo, and return None
@@ -79,8 +79,13 @@ return None
 SELECT invalid_type_caught('rick');
 WARNING:  PL/Python: plpy.SPIError: unrecognized error in PLy_spi_prepare
 CONTEXT:  PL/Python function "invalid_type_caught"
-ERROR:  type "test" does not exist
+NOTICE:  type "test" does not exist
 CONTEXT:  PL/Python function "invalid_type_caught"
+ invalid_type_caught 
+---------------------
+ 
+(1 row)
+
 /* for what it's worth catch the exception generated by
  * the typo, and reraise it as a plain error
  */
@@ -101,7 +106,7 @@ return None
 SELECT invalid_type_reraised('rick');
 WARNING:  PL/Python: plpy.SPIError: unrecognized error in PLy_spi_prepare
 CONTEXT:  PL/Python function "invalid_type_reraised"
-ERROR:  type "test" does not exist
+ERROR:  PL/Python: plpy.Error: type "test" does not exist
 CONTEXT:  PL/Python function "invalid_type_reraised"
 /* no typo no messing about
  */

diff --git a/src/pl/plpython/expected/plpython_test.out b/src/pl/plpython/expected/plpython_test.out
index 3293829..583a149 100644 (file)
--- a/src/pl/plpython/expected/plpython_test.out
+++ b/src/pl/plpython/expected/plpython_test.out
@@ -73,5 +73,5 @@ NOTICE:  notice
 CONTEXT:  PL/Python function "elog_test"
 WARNING:  warning
 CONTEXT:  PL/Python function "elog_test"
-ERROR:  error
+ERROR:  PL/Python: plpy.Error: error
 CONTEXT:  PL/Python function "elog_test"

diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 18a523c..d1fac11 100644 (file)
--- a/src/pl/plpython/plpython.c
+++ b/src/pl/plpython/plpython.c
@@ -284,6 +284,9 @@ PLy_exception_set_plural(PyObject *, const char *, const char *,
 __attribute__((format(printf, 2, 5)))
 __attribute__((format(printf, 3, 5)));
 
+/* like PLy_exception_set, but conserve more fields from ErrorData */
+static void PLy_spi_exception_set(ErrorData *edata);
+
 /* Get the innermost python procedure called from the backend */
 static char *PLy_procedure_name(PLyProcedure *);
 
@@ -291,6 +294,7 @@ static char *PLy_procedure_name(PLyProcedure *);
 static void
 PLy_elog(int, const char *,...)
 __attribute__((format(printf, 2, 3)));
+static void PLy_get_spi_error_data(PyObject *exc, char **hint, char **query, int *position);
 static char *PLy_traceback(int *);
 
 static void *PLy_malloc(size_t);
@@ -364,19 +368,6 @@ static HeapTuple PLyObject_ToTuple(PLyTypeInfo *, PyObject *);
  */
 static PLyProcedure *PLy_curr_procedure = NULL;
 
-/*
- * When a callback from Python into PG incurs an error, we temporarily store
- * the error information here, and return NULL to the Python interpreter.
- * Any further callback attempts immediately fail, and when the Python
- * interpreter returns to the calling function, we re-throw the error (even if
- * Python thinks it trapped the error and doesn't return NULL).  Eventually
- * this ought to be improved to let Python code really truly trap the error,
- * but that's more of a change from the pre-8.0 semantics than I have time for
- * now --- it will only be possible if the callback query is executed inside a
- * subtransaction.
- */
-static ErrorData *PLy_error_in_progress = NULL;
-
 static PyObject *PLy_interp_globals = NULL;
 static PyObject *PLy_interp_safe_globals = NULL;
 static HTAB *PLy_procedure_cache = NULL;
@@ -597,7 +588,6 @@ PLy_trigger_handler(FunctionCallInfo fcinfo, PLyProcedure *proc)
                plrv = PLy_procedure_call(proc, "TD", plargs);
 
                Assert(plrv != NULL);
-               Assert(!PLy_error_in_progress);
 
                /*
                 * Disconnect from SPI manager
@@ -1015,7 +1005,6 @@ PLy_function_handler(FunctionCallInfo fcinfo, PLyProcedure *proc)
                                PLy_function_delete_args(proc);
                        }
                        Assert(plrv != NULL);
-                       Assert(!PLy_error_in_progress);
                }
 
                /*
@@ -1194,23 +1183,9 @@ PLy_procedure_call(PLyProcedure *proc, char *kargs, PyObject *vargs)
        rv = PyEval_EvalCode((PyCodeObject *) proc->code,
                                                 proc->globals, proc->globals);
 
-       /*
-        * If there was an error in a PG callback, propagate that no matter what
-        * Python claims about its success.
-        */
-       if (PLy_error_in_progress)
-       {
-               ErrorData  *edata = PLy_error_in_progress;
-
-               PLy_error_in_progress = NULL;
-               ReThrowError(edata);
-       }
-
-       if (rv == NULL || PyErr_Occurred())
-       {
-               Py_XDECREF(rv);
+       /* If the Python code returned an error, propagate it */
+       if (rv == NULL)
                PLy_elog(ERROR, NULL);
-       }
 
        return rv;
 }
@@ -2862,13 +2837,6 @@ PLy_spi_prepare(PyObject *self, PyObject *args)
        void       *tmpplan;
        volatile MemoryContext oldcontext;
 
-       /* Can't execute more if we have an unhandled error */
-       if (PLy_error_in_progress)
-       {
-               PLy_exception_set(PLy_exc_error, "transaction aborted");
-               return NULL;
-       }
-
        if (!PyArg_ParseTuple(args, "s|O", &query, &list))
        {
                PLy_exception_set(PLy_exc_spi_error,
@@ -2978,8 +2946,10 @@ PLy_spi_prepare(PyObject *self, PyObject *args)
        }
        PG_CATCH();
        {
+               ErrorData       *edata;
+
                MemoryContextSwitchTo(oldcontext);
-               PLy_error_in_progress = CopyErrorData();
+               edata = CopyErrorData();
                FlushErrorState();
                Py_DECREF(plan);
                Py_XDECREF(optr);
@@ -2987,6 +2957,7 @@ PLy_spi_prepare(PyObject *self, PyObject *args)
                        PLy_exception_set(PLy_exc_spi_error,
                                                          "unrecognized error in PLy_spi_prepare");
                PLy_elog(WARNING, NULL);
+               PLy_spi_exception_set(edata);
                return NULL;
        }
        PG_END_TRY();
@@ -3005,13 +2976,6 @@ PLy_spi_execute(PyObject *self, PyObject *args)
        PyObject   *list = NULL;
        long            limit = 0;
 
-       /* Can't execute more if we have an unhandled error */
-       if (PLy_error_in_progress)
-       {
-               PLy_exception_set(PLy_exc_error, "transaction aborted");
-               return NULL;
-       }
-
        if (PyArg_ParseTuple(args, "s|l", &query, &limit))
                return PLy_spi_execute_query(query, limit);
 
@@ -3116,9 +3080,10 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
        PG_CATCH();
        {
                int                     k;
+               ErrorData       *edata;
 
                MemoryContextSwitchTo(oldcontext);
-               PLy_error_in_progress = CopyErrorData();
+               edata = CopyErrorData();
                FlushErrorState();
 
                /*
@@ -3138,6 +3103,7 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
                        PLy_exception_set(PLy_exc_error,
                                                          "unrecognized error in PLy_spi_execute_plan");
                PLy_elog(WARNING, NULL);
+               PLy_spi_exception_set(edata);
                return NULL;
        }
        PG_END_TRY();
@@ -3152,14 +3118,6 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit)
                }
        }
 
-       if (rv < 0)
-       {
-               PLy_exception_set(PLy_exc_spi_error,
-                                                 "SPI_execute_plan failed: %s",
-                                                 SPI_result_code_string(rv));
-               return NULL;
-       }
-
        return PLy_spi_execute_fetch_result(SPI_tuptable, SPI_processed, rv);
 }
 
@@ -3177,13 +3135,16 @@ PLy_spi_execute_query(char *query, long limit)
        }
        PG_CATCH();
        {
+               ErrorData       *edata;
+
                MemoryContextSwitchTo(oldcontext);
-               PLy_error_in_progress = CopyErrorData();
+               edata = CopyErrorData();
                FlushErrorState();
                if (!PyErr_Occurred())
                        PLy_exception_set(PLy_exc_spi_error,
                                                          "unrecognized error in PLy_spi_execute_query");
                PLy_elog(WARNING, NULL);
+               PLy_spi_exception_set(edata);
                return NULL;
        }
        PG_END_TRY();
@@ -3244,8 +3205,6 @@ PLy_spi_execute_fetch_result(SPITupleTable *tuptable, int rows, int status)
                PG_CATCH();
                {
                        MemoryContextSwitchTo(oldcontext);
-                       PLy_error_in_progress = CopyErrorData();
-                       FlushErrorState();
                        if (!PyErr_Occurred())
                                PLy_exception_set(PLy_exc_error,
                                           "unrecognized error in PLy_spi_execute_fetch_result");
@@ -3502,23 +3461,20 @@ PLy_output(volatile int level, PyObject *self, PyObject *args)
        }
        PG_CATCH();
        {
+               ErrorData  *edata;
+
                MemoryContextSwitchTo(oldcontext);
-               PLy_error_in_progress = CopyErrorData();
+               edata = CopyErrorData();
                FlushErrorState();
 
-               PyErr_SetString(PLy_exc_error, sv);
-
                /*
                 * Note: If sv came from PyString_AsString(), it points into storage
                 * owned by so.  So free so after using sv.
                 */
                Py_XDECREF(so);
 
-               /*
-                * returning NULL here causes the python interpreter to bail. when
-                * control passes back to PLy_procedure_call, we check for PG
-                * exceptions and re-throw the error.
-                */
+               /* Make Python raise the exception */
+               PLy_exception_set(PLy_exc_error, edata->message);
                return NULL;
        }
        PG_END_TRY();
@@ -3584,6 +3540,48 @@ PLy_exception_set_plural(PyObject *exc,
        PyErr_SetString(exc, buf);
 }
 
+/*
+ * Raise a SPIError, passing in it more error details, like the
+ * internal query and error position.
+ */
+static void
+PLy_spi_exception_set(ErrorData *edata)
+{
+       PyObject        *args = NULL;
+       PyObject        *spierror = NULL;
+       PyObject        *spidata = NULL;
+
+       args = Py_BuildValue("(s)", edata->message);
+       if (!args)
+               goto failure;
+
+       /* create a new SPIError with the error message as the parameter */
+       spierror = PyObject_CallObject(PLy_exc_spi_error, args);
+       if (!spierror)
+               goto failure;
+
+       spidata = Py_BuildValue("(zzi)", edata->hint,
+                                                       edata->internalquery, edata->internalpos);
+       if (!spidata)
+               goto failure;
+
+       if (PyObject_SetAttrString(spierror, "spidata", spidata) == -1)
+               goto failure;
+
+       PyErr_SetObject(PLy_exc_spi_error, spierror);
+
+       Py_DECREF(args);
+       Py_DECREF(spierror);
+       Py_DECREF(spidata);
+       return;
+
+failure:
+       Py_XDECREF(args);
+       Py_XDECREF(spierror);
+       Py_XDECREF(spidata);
+       elog(ERROR, "could not convert SPI error to Python exception");
+}
+
 /* Emit a PG error or notice, together with any available info about
  * the current Python error, previously set by PLy_exception_set().
  * This should be used to propagate Python errors into PG.     If fmt is
@@ -3596,6 +3594,15 @@ PLy_elog(int elevel, const char *fmt,...)
        char       *xmsg;
        int                     xlevel;
        StringInfoData emsg;
+       PyObject        *exc, *val, *tb;
+       char            *hint = NULL;
+       char            *query = NULL;
+       int                     position = 0;
+
+       PyErr_Fetch(&exc, &val, &tb);
+       if (exc != NULL && PyErr_GivenExceptionMatches(val, PLy_exc_spi_error))
+               PLy_get_spi_error_data(val, &hint, &query, &position);
+       PyErr_Restore(exc, val, tb);
 
        xmsg = PLy_traceback(&xlevel);
 
@@ -3621,10 +3628,16 @@ PLy_elog(int elevel, const char *fmt,...)
                if (fmt)
                        ereport(elevel,
                                        (errmsg("PL/Python: %s", emsg.data),
-                                        (xmsg) ? errdetail("%s", xmsg) : 0));
+                                        (xmsg) ? errdetail("%s", xmsg) : 0,
+                                        (hint) ? errhint(hint) : 0,
+                                        (query) ? internalerrquery(query) : 0,
+                                        (position) ? internalerrposition(position) : 0));
                else
                        ereport(elevel,
-                                       (errmsg("PL/Python: %s", xmsg)));
+                                       (errmsg("PL/Python: %s", xmsg),
+                                        (hint) ? errhint(hint) : 0,
+                                        (query) ? internalerrquery(query) : 0,
+                                        (position) ? internalerrposition(position) : 0));
        }
        PG_CATCH();
        {
@@ -3642,6 +3655,28 @@ PLy_elog(int elevel, const char *fmt,...)
                pfree(xmsg);
 }
 
+/*
+ * Extract the error data from a SPIError
+ */
+static void
+PLy_get_spi_error_data(PyObject *exc, char **hint, char **query, int *position)
+{
+       PyObject        *spidata = NULL;
+
+       spidata = PyObject_GetAttrString(exc, "spidata");
+       if (!spidata)
+               goto cleanup;
+
+       if (!PyArg_ParseTuple(spidata, "zzi", hint, query, position))
+               goto cleanup;
+
+cleanup:
+       PyErr_Clear();
+       /* no elog here, we simply won't report the errhint, errposition etc */
+       Py_XDECREF(spidata);
+}
+
+
 static char *
 PLy_traceback(int *xlevel)
 {