On 26/01/11 04:51, Steve Singer wrote:
> On 10-12-23 08:45 AM, Jan Urbański wrote:
> I see you've merged the changes from the refactoring branch down but
> haven't yet posted an updated patch. This review is based on
> 2f2b4a33bf344058620a5c684d1f2459e505c727
Thanks for the review, I'm attaching an updated patch against master.
> The patch updates the excepted results of the regression tests so they
> no longer expect the 'unrecognized error' warnings. No new unit test
> are added to verify that behavior changes will continue to function as
> intended (though they could be)
It's in fact just a correctness change, so I didn't include any new unit
tests.
> No documentation updates are included. The current documentation is
> silent on the behaviour of plpython when SPI calls generate errors so
> this change doesn't invalidate any documentation but it would be nice if
> we described what effect SQL invoked through SPI from the functions have
> on the transaction. Maybe a "Trapping Errors" section?
Good point, the fact that you can now actually catch SPI errors should
be documented, I'll try to add a paragraph about that.
> A concern I have is that some users might find this surprising. For
> plpgsql the exception handling will rollback SQL from before the
> exception and I suspect the other PL's are the same. It would be good
> if a few more people chimed in on if they see this as a good idea.
It's not true for other PLs, but see below.
> Another concern is that we are probably breaking some peoples code.
>
> Consider the following:
>
> BEGIN;
> create table foo(a int4 primary key);
> DO $$
> r=plpy.execute("insert into foo values ('1')")
> try :
> r=plpy.execute("insert into foo values ('1')")
> except:
> plpy.log("something went wrong")
> $$ language plpython2u;
> select * FROM foo;
> a
> ---
> 1
> (1 row)
>
>
> This code is going to behave different with the patch.
Right, without the patch you can never catch errors originating from
plpy.execute, so any error terminates the whole function, and so rolls
back the statement. FWIW PL/Perl works the same:
begin;
create table foo(i int primary key);
DO $$
spi_exec_query("insert into foo values ('1')");
eval { spi_exec_query("insert into foo values ('1')"); };
elog(LOG, $@) if $@;
$$ language plperl;
select * from foo;
You will see a row in foo. AFAICS PL/Tcl also does it, but I don't have
it complied it to try. It does consitute a behaviour change, but we
didn't get any complains when the same thing happened for Perl.
> I am finding the treatment of savepoints very strange.
> If as a function author I'm able to recover from errors then I'd expect
> (or maybe want) to be able to manage them through savepoints
Ooops, you found a bug there. In the attached patch you get the same
error (SPI_ERROR_TRANSACTION) as in master. Also added a unit test for that.
> I've only glanced at your transaction manager patch, from what I can
> tell it will give me another way of managing the inner transaction but
> I'm not sure it will make the savepoint calls work(will it?). I also
> wonder how useful in practice this patch will be if the other patch
> doesn't also get applied (function others will be stuck with an all or
> nothing as their options for error handling)
As long as you don't catch SPIError, nothing changes. And error in a SPI
call will propagate to the top of the Python stack and your function
will be terminated, its effects being rolled back. The function will
only work differently if you have bare except: clauses (that are
deprecated in 2.x and forbidden in 3.x IIRC) or catch Exception.
For example:
begin;
create table foo(i int primary key);
DO $$
plpy.execute("insert into foo values ('1')")
plpy.execute("insert into foo values ('1')")
$$ language plpython2u;
No row will be inserted and the whole transaction will be rolled back.
As for explicit subxact management, that's something that would be
unique to PL/Python, and my other patch implements it like this:
begin;
create table foo(i int primary key);
DO $$
try:
with plpy.subxact():
plpy.execute("insert into foo values ('1')")
plpy.execute("insert into foo values ('1')")
except plpy.SPIError:
plpy.log("no rows inserted")
$$ language plpython2u;
Thanks again for the review and for spotting that bug!
Cheers,
Jan
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index ea4a54c..4eeda6f 100644
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*************** CREATE FUNCTION sql_syntax_error() RETUR
*** 8,15 ****
'plpy.execute("syntax error")'
LANGUAGE plpythonu;
SELECT sql_syntax_error();
- WARNING: plpy.SPIError: unrecognized error in PLy_spi_execute_query
- CONTEXT: PL/Python function "sql_syntax_error"
ERROR: plpy.SPIError: syntax error at or near "syntax"
LINE 1: syntax error
^
--- 8,13 ----
*************** CREATE FUNCTION exception_index_invalid_
*** 32,39 ****
return rv[0]'
LANGUAGE plpythonu;
SELECT exception_index_invalid_nested();
- WARNING: plpy.SPIError: unrecognized error in PLy_spi_execute_query
- CONTEXT: PL/Python function "exception_index_invalid_nested"
ERROR: plpy.SPIError: function test5(unknown) does not exist
LINE 1: SELECT test5('foo')
^
--- 30,35 ----
*************** return None
*** 54,61 ****
'
LANGUAGE plpythonu;
SELECT invalid_type_uncaught('rick');
- WARNING: plpy.SPIError: unrecognized error in PLy_spi_prepare
- CONTEXT: PL/Python function "invalid_type_uncaught"
ERROR: plpy.SPIError: type "test" does not exist
CONTEXT: PL/Python function "invalid_type_uncaught"
/* for what it's worth catch the exception generated by
--- 50,55 ----
*************** return None
*** 77,84 ****
'
LANGUAGE plpythonu;
SELECT invalid_type_caught('rick');
- WARNING: plpy.SPIError: unrecognized error in PLy_spi_prepare
- CONTEXT: PL/Python function "invalid_type_caught"
NOTICE: type "test" does not exist
CONTEXT: PL/Python function "invalid_type_caught"
invalid_type_caught
--- 71,76 ----
*************** return None
*** 104,111 ****
'
LANGUAGE plpythonu;
SELECT invalid_type_reraised('rick');
- WARNING: plpy.SPIError: unrecognized error in PLy_spi_prepare
- CONTEXT: PL/Python function "invalid_type_reraised"
ERROR: plpy.Error: type "test" does not exist
CONTEXT: PL/Python function "invalid_type_reraised"
/* no typo no messing about
--- 96,101 ----
*************** SELECT valid_type('rick');
*** 126,128 ****
--- 116,128 ----
(1 row)
+ /* manually starting subtransactions - a bad idea
+ */
+ CREATE FUNCTION manual_subxact() RETURNS void AS $$
+ plpy.execute("savepoint save")
+ plpy.execute("create table foo(x integer)")
+ plpy.execute("rollback to save")
+ $$ LANGUAGE plpythonu;
+ SELECT manual_subxact();
+ ERROR: plpy.SPIError: SPI_execute failed: SPI_ERROR_TRANSACTION
+ CONTEXT: PL/Python function "manual_subxact"
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index aafe556..3d083e1 100644
*** a/src/pl/plpython/plpython.c
--- b/src/pl/plpython/plpython.c
*************** typedef int Py_ssize_t;
*** 101,106 ****
--- 101,107 ----
#include "nodes/makefuncs.h"
#include "parser/parse_type.h"
#include "tcop/tcopprot.h"
+ #include "access/xact.h"
#include "utils/builtins.h"
#include "utils/hsearch.h"
#include "utils/lsyscache.h"
*************** PLy_spi_prepare(PyObject *self, PyObject
*** 2817,2822 ****
--- 2818,2824 ----
char *query;
void *tmpplan;
volatile MemoryContext oldcontext;
+ volatile ResourceOwner oldowner;
int nargs;
if (!PyArg_ParseTuple(args, "s|O", &query, &list))
*************** PLy_spi_prepare(PyObject *self, PyObject
*** 2840,2845 ****
--- 2842,2852 ----
plan->args = nargs ? PLy_malloc(sizeof(PLyTypeInfo) * nargs) : NULL;
oldcontext = CurrentMemoryContext;
+ oldowner = CurrentResourceOwner;
+
+ BeginInternalSubTransaction(NULL);
+ MemoryContextSwitchTo(oldcontext);
+
PG_TRY();
{
int i;
*************** PLy_spi_prepare(PyObject *self, PyObject
*** 2919,2938 ****
if (plan->plan == NULL)
elog(ERROR, "SPI_saveplan failed: %s",
SPI_result_code_string(SPI_result));
}
PG_CATCH();
{
ErrorData *edata;
MemoryContextSwitchTo(oldcontext);
edata = CopyErrorData();
FlushErrorState();
Py_DECREF(plan);
Py_XDECREF(optr);
! if (!PyErr_Occurred())
! PLy_exception_set(PLy_exc_spi_error,
! "unrecognized error in PLy_spi_prepare");
! PLy_elog(WARNING, NULL);
PLy_spi_exception_set(edata);
return NULL;
}
--- 2926,2967 ----
if (plan->plan == NULL)
elog(ERROR, "SPI_saveplan failed: %s",
SPI_result_code_string(SPI_result));
+
+ /* Commit the inner transaction, return to outer xact context */
+ ReleaseCurrentSubTransaction();
+ MemoryContextSwitchTo(oldcontext);
+ CurrentResourceOwner = oldowner;
+
+ /*
+ * AtEOSubXact_SPI() should not have popped any SPI context, but just
+ * in case it did, make sure we remain connected.
+ */
+ SPI_restore_connection();
}
PG_CATCH();
{
ErrorData *edata;
+ /* Save error info */
MemoryContextSwitchTo(oldcontext);
edata = CopyErrorData();
FlushErrorState();
Py_DECREF(plan);
Py_XDECREF(optr);
!
! /* Abort the inner transaction */
! RollbackAndReleaseCurrentSubTransaction();
! MemoryContextSwitchTo(oldcontext);
! CurrentResourceOwner = oldowner;
!
! /*
! * If AtEOSubXact_SPI() popped any SPI context of the subxact, it
! * will have left us in a disconnected state. We need this hack to
! * return to connected state.
! */
! SPI_restore_connection();
!
! /* Make Python raise the exception */
PLy_spi_exception_set(edata);
return NULL;
}
*************** PLy_spi_execute_plan(PyObject *ob, PyObj
*** 2974,2979 ****
--- 3003,3009 ----
rv;
PLyPlanObject *plan;
volatile MemoryContext oldcontext;
+ volatile ResourceOwner oldowner;
PyObject *ret;
if (list != NULL)
*************** PLy_spi_execute_plan(PyObject *ob, PyObj
*** 3009,3014 ****
--- 3039,3050 ----
}
oldcontext = CurrentMemoryContext;
+ oldowner = CurrentResourceOwner;
+
+ BeginInternalSubTransaction(NULL);
+ /* Want to run inside function's memory context */
+ MemoryContextSwitchTo(oldcontext);
+
PG_TRY();
{
char *nulls;
*************** PLy_spi_execute_plan(PyObject *ob, PyObj
*** 3061,3072 ****
--- 3097,3120 ----
if (nargs > 0)
pfree(nulls);
+
+ /* Commit the inner transaction, return to outer xact context */
+ ReleaseCurrentSubTransaction();
+ MemoryContextSwitchTo(oldcontext);
+ CurrentResourceOwner = oldowner;
+
+ /*
+ * AtEOSubXact_SPI() should not have popped any SPI context, but just
+ * in case it did, make sure we remain connected.
+ */
+ SPI_restore_connection();
}
PG_CATCH();
{
int k;
ErrorData *edata;
+ /* Save error info */
MemoryContextSwitchTo(oldcontext);
edata = CopyErrorData();
FlushErrorState();
*************** PLy_spi_execute_plan(PyObject *ob, PyObj
*** 3084,3093 ****
}
}
! if (!PyErr_Occurred())
! PLy_exception_set(PLy_exc_spi_error,
! "unrecognized error in PLy_spi_execute_plan");
! PLy_elog(WARNING, NULL);
PLy_spi_exception_set(edata);
return NULL;
}
--- 3132,3150 ----
}
}
! /* Abort the inner transaction */
! RollbackAndReleaseCurrentSubTransaction();
! MemoryContextSwitchTo(oldcontext);
! CurrentResourceOwner = oldowner;
!
! /*
! * If AtEOSubXact_SPI() popped any SPI context of the subxact, it
! * will have left us in a disconnected state. We need this hack to
! * return to connected state.
! */
! SPI_restore_connection();
!
! /* Make Python raise the exception */
PLy_spi_exception_set(edata);
return NULL;
}
*************** PLy_spi_execute_query(char *query, long
*** 3111,3136 ****
{
int rv;
volatile MemoryContext oldcontext;
PyObject *ret;
oldcontext = CurrentMemoryContext;
PG_TRY();
{
pg_verifymbstr(query, strlen(query), false);
rv = SPI_execute(query, PLy_curr_procedure->fn_readonly, limit);
ret = PLy_spi_execute_fetch_result(SPI_tuptable, SPI_processed, rv);
}
PG_CATCH();
{
ErrorData *edata;
MemoryContextSwitchTo(oldcontext);
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;
}
--- 3168,3222 ----
{
int rv;
volatile MemoryContext oldcontext;
+ volatile ResourceOwner oldowner;
PyObject *ret;
oldcontext = CurrentMemoryContext;
+ oldowner = CurrentResourceOwner;
+
+ BeginInternalSubTransaction(NULL);
+ /* Want to run inside function's memory context */
+ MemoryContextSwitchTo(oldcontext);
+
PG_TRY();
{
pg_verifymbstr(query, strlen(query), false);
rv = SPI_execute(query, PLy_curr_procedure->fn_readonly, limit);
ret = PLy_spi_execute_fetch_result(SPI_tuptable, SPI_processed, rv);
+
+ /* Commit the inner transaction, return to outer xact context */
+ ReleaseCurrentSubTransaction();
+ MemoryContextSwitchTo(oldcontext);
+ CurrentResourceOwner = oldowner;
+
+ /*
+ * AtEOSubXact_SPI() should not have popped any SPI context, but just
+ * in case it did, make sure we remain connected.
+ */
+ SPI_restore_connection();
}
PG_CATCH();
{
ErrorData *edata;
+ /* Save error info */
MemoryContextSwitchTo(oldcontext);
edata = CopyErrorData();
FlushErrorState();
!
! /* Abort the inner transaction */
! RollbackAndReleaseCurrentSubTransaction();
! MemoryContextSwitchTo(oldcontext);
! CurrentResourceOwner = oldowner;
!
! /*
! * If AtEOSubXact_SPI() popped any SPI context of the subxact, it
! * will have left us in a disconnected state. We need this hack to
! * return to connected state.
! */
! SPI_restore_connection();
!
! /* Make Python raise the exception */
PLy_spi_exception_set(edata);
return NULL;
}
diff --git a/src/pl/plpython/sql/plpython_error.sql b/src/pl/plpython/sql/plpython_error.sql
index 5ca6849..876a8d6 100644
*** a/src/pl/plpython/sql/plpython_error.sql
--- b/src/pl/plpython/sql/plpython_error.sql
*************** return None
*** 107,109 ****
--- 107,119 ----
LANGUAGE plpythonu;
SELECT valid_type('rick');
+
+ /* manually starting subtransactions - a bad idea
+ */
+ CREATE FUNCTION manual_subxact() RETURNS void AS $$
+ plpy.execute("savepoint save")
+ plpy.execute("create table foo(x integer)")
+ plpy.execute("rollback to save")
+ $$ LANGUAGE plpythonu;
+
+ SELECT manual_subxact();
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
