Re: [HACKERS] pl/python SPI in subtransactions
You mentioned that you were going to add a few paragraphs to the documentation saying that you can now actually catch SPI errors. I haven't seen that yet. Yeah, I'm procrastinating the doc writing part ;) Will spit something out by the end of the (CET) day. Done, added a small example in the Database Access section. Committed. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pl/python SPI in subtransactions
On 29/01/11 22:13, Jan Urbański wrote:
On 29/01/11 22:10, Steve Singer wrote:
On 11-01-29 03:39 PM, Jan Urbański wrote:
D'oh, you're right, thanks. Attached patch with fix. Curiosly, right now
in master your example with plpy.prepare will result in savepoint
being swallowed, but it's of course better to react with an error.
Cheers,
Jan
This seems to fix it.
You mentioned that you were going to add a few paragraphs to the
documentation saying that you can now actually catch SPI errors. I
haven't seen that yet.
Yeah, I'm procrastinating the doc writing part ;) Will spit something
out by the end of the (CET) day.
Done, added a small example in the Database Access section.
Cheers,
Jan
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
index 9fc8808..feb5722 100644
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 858,863
--- 858,866
literalplpy.replaceablefoo/replaceable/literal.
/para
+ sect2
+ titleDatabase access functions/title
+
para
The literalplpy/literal module provides two
functions called functionexecute/function and
*** CREATE FUNCTION usesavedplan() RETURNS t
*** 937,942
--- 940,972
$$ LANGUAGE plpythonu;
/programlisting
/para
+
+ /sect2
+
+ sect2
+ titleTrapping Errors/title
+
+ para
+Functions accessing the database might encounter errors, which will cause
+them to abort and raise an exception. Both functionplpy.execute/function
+and functionplpy.prepare/function can raise an instance
+of literalplpy.SPIError/literal, which by default will terminate the
+function. This error can be handled just like any other Python exception,
+by using the literaltry/except/literal construct. For example:
+ programlisting
+ CREATE FUNCTION try_adding_joe() RETURNS text AS $$
+ try:
+ plpy.execute(INSERT INTO users(username) VALUES ('joe'))
+ except plpy.SPIError:
+ return something went wrong
+ else:
+ return Joe added
+ $$ LANGUAGE plpythonu;
+ /programlisting
+ /para
+
+ /sect2
+
/sect1
sect1 id=plpython-util
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index ea4a54c..c8ce984 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,140
(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
+ /* same for prepared plans
+ */
+ CREATE FUNCTION manual_subxact_prepared() RETURNS void AS $$
+ save = plpy.prepare(savepoint save)
+ rollback =
Re: [HACKERS] pl/python SPI in subtransactions
On 31/01/11 00:03, Jan Urbański wrote:
On 29/01/11 22:13, Jan Urbański wrote:
On 29/01/11 22:10, Steve Singer wrote:
On 11-01-29 03:39 PM, Jan Urbański wrote:
D'oh, you're right, thanks. Attached patch with fix. Curiosly, right now
in master your example with plpy.prepare will result in savepoint
being swallowed, but it's of course better to react with an error.
Cheers,
Jan
This seems to fix it.
You mentioned that you were going to add a few paragraphs to the
documentation saying that you can now actually catch SPI errors. I
haven't seen that yet.
Yeah, I'm procrastinating the doc writing part ;) Will spit something
out by the end of the (CET) day.
Done, added a small example in the Database Access section.
... aaand another one. I'll never get used to the way section titles are
capitalized.
Jan
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
index 9fc8808..324e606 100644
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 858,863
--- 858,866
literalplpy.replaceablefoo/replaceable/literal.
/para
+ sect2
+ titleDatabase Access Functions/title
+
para
The literalplpy/literal module provides two
functions called functionexecute/function and
*** CREATE FUNCTION usesavedplan() RETURNS t
*** 937,942
--- 940,972
$$ LANGUAGE plpythonu;
/programlisting
/para
+
+ /sect2
+
+ sect2
+ titleTrapping Errors/title
+
+ para
+Functions accessing the database might encounter errors, which will cause
+them to abort and raise an exception. Both functionplpy.execute/function
+and functionplpy.prepare/function can raise an instance
+of literalplpy.SPIError/literal, which by default will terminate the
+function. This error can be handled just like any other Python exception,
+by using the literaltry/except/literal construct. For example:
+ programlisting
+ CREATE FUNCTION try_adding_joe() RETURNS text AS $$
+ try:
+ plpy.execute(INSERT INTO users(username) VALUES ('joe'))
+ except plpy.SPIError:
+ return something went wrong
+ else:
+ return Joe added
+ $$ LANGUAGE plpythonu;
+ /programlisting
+ /para
+
+ /sect2
+
/sect1
sect1 id=plpython-util
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index ea4a54c..c8ce984 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,140
(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
+ /* same for prepared plans
+ */
+
Re: [HACKERS] pl/python SPI in subtransactions
On 11-01-27 04:33 PM, Jan Urbański wrote:
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.
If we've made this type of behaviour change for pl/perl and no one
complained then I don't see an issue with doing it for plpython (if
anyone does they should speak up)
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 think you need to make the same change to PLy_spi_execute_plan.
Try
BEGIN;
create table foo(a int4 primary key);
DO $$
prep=plpy.prepare(savepoint save)
plpy.execute(prep)
r=plpy.execute(insert into foo values ('1'))
try :
r=plpy.execute(insert into foo values ('1'))
except:
prep=plpy.prepare(rollback to save)
plpy.execute(prep)
plpy.log(something went wrong)
$$ language plpythonu;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pl/python SPI in subtransactions
On 29/01/11 21:27, Steve Singer wrote:
On 11-01-27 04:33 PM, Jan Urbański wrote:
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 think you need to make the same change to PLy_spi_execute_plan.
D'oh, you're right, thanks. Attached patch with fix. Curiosly, right now
in master your example with plpy.prepare will result in savepoint
being swallowed, but it's of course better to react with an error.
Cheers,
Jan
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index ea4a54c..c8ce984 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,140
(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
+ /* same for prepared plans
+ */
+ CREATE FUNCTION manual_subxact_prepared() RETURNS void AS $$
+ save = plpy.prepare(savepoint save)
+ rollback = plpy.prepare(rollback to save)
+ plpy.execute(save)
+ plpy.execute(create table foo(x integer))
+ plpy.execute(rollback)
+ $$ LANGUAGE plpythonu;
+ SELECT manual_subxact_prepared();
+ ERROR: plpy.SPIError: SPI_execute_plan failed: SPI_ERROR_TRANSACTION
+ CONTEXT: PL/Python function manual_subxact_prepared
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index aafe556..6d3566a 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,
Re: [HACKERS] pl/python SPI in subtransactions
On 11-01-29 03:39 PM, Jan Urbański wrote: D'oh, you're right, thanks. Attached patch with fix. Curiosly, right now in master your example with plpy.prepare will result in savepoint being swallowed, but it's of course better to react with an error. Cheers, Jan This seems to fix it. You mentioned that you were going to add a few paragraphs to the documentation saying that you can now actually catch SPI errors. I haven't seen that yet. Other than that I don't see any issues with the patch and it should be ready for a committer. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pl/python SPI in subtransactions
On 29/01/11 22:10, Steve Singer wrote: On 11-01-29 03:39 PM, Jan Urbański wrote: D'oh, you're right, thanks. Attached patch with fix. Curiosly, right now in master your example with plpy.prepare will result in savepoint being swallowed, but it's of course better to react with an error. Cheers, Jan This seems to fix it. You mentioned that you were going to add a few paragraphs to the documentation saying that you can now actually catch SPI errors. I haven't seen that yet. Yeah, I'm procrastinating the doc writing part ;) Will spit something out by the end of the (CET) day. Other than that I don't see any issues with the patch and it should be ready for a committer. Great, thanks for a diligent review, Jan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pl/python SPI in subtransactions
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
Re: [HACKERS] pl/python SPI in subtransactions
On 10-12-23 08:45 AM, Jan Urbański wrote:
Here's a patch implementing a executing SPI in an subtransaction
mentioned in
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
an incremental patch on top of the plpython-refactor patch sent eariler.
Git branch for this patch:
https://github.com/wulczer/postgres/tree/spi-in-subxacts.
Without it the error handling in PL/Python is really broken, as we jump
between from a saught longjmp back into Python without any cleanup. As
an additional bonus you no longer get all the ugly unrecognized error
in PLy_spi_execute_query errors.
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
As a disclaimer, I have worked python before but not used plpython for
anything substantial.
Submission Review
---
I think Jan intends to post an updated patch once the refactor branch
has been dealt with.
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)
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?
Usability Review
---
Does the patch implement what it says: yes
Do we want this: yes I think so. This patch allows pl/python functions
to catch errors from the SQL they issue and deal with them as the
function author sees fit. I see this being useful.
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.
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. Without the
patch the select fails because a) the transaction has rolled back which
rollsback both insert and the create table. With the patch the first
row shows up in the select. How concerned are we with changing the
behaviour of existing plpython functions? This needs discussion.
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
BEGIN;
create table foo(a int4 primary key);
DO $$
plpy.execute(savepoint save)
r=plpy.execute(insert into foo values ('1'))
try :
r=plpy.execute(insert into foo values ('1'))
except:
plpy.execute(rollback to save)
plpy.log(something went wrong)
$$ language plpython2u;
select * FROM foo;
a
---
1
(1 row)
when I wrote the above I was expecting either an error when I tried to
use the savepoint (like in plpgsql) or for it rollback the insert.
Without the patch I get
PL/Python: plpy.SPIError: SPI_execute failed: SPI_ERROR_TRANSACTION
This is much better than silently ignoring the command.
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)
Code Review
---
I don't see any issues with the code.
Cheers,
Jan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pl/python SPI in subtransactions
Here's a patch implementing a executing SPI in an subtransaction
mentioned in
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
an incremental patch on top of the plpython-refactor patch sent eariler.
Git branch for this patch:
https://github.com/wulczer/postgres/tree/spi-in-subxacts.
Without it the error handling in PL/Python is really broken, as we jump
between from a saught longjmp back into Python without any cleanup. As
an additional bonus you no longer get all the ugly unrecognized error
in PLy_spi_execute_query errors.
Cheers,
Jan
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index 70890a8..7fc8337 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
CONTEXT: PL/Python function sql_syntax_error
/* check the handling of uncaught python exceptions
--- 8,13
*** CREATE FUNCTION exception_index_invalid_
*** 29,36
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
CONTEXT: PL/Python function exception_index_invalid_nested
/* a typo
--- 27,32
*** return None
*** 47,54
'
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
--- 43,48
*** return None
*** 70,77
'
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
--- 64,69
*** return None
*** 97,104
'
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
--- 89,94
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 67eb0f3..5b216b2 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/lsyscache.h
#include utils/memutils.h
*** PLy_spi_prepare(PyObject *self, PyObject
*** 2829,2834
--- 2830,2836
int nargs;
MemoryContext volatile oldcontext = CurrentMemoryContext;
+ ResourceOwner volatile oldowner = CurrentResourceOwner;
if (!PyArg_ParseTuple(args, s|O, query, list))
{
*** PLy_spi_prepare(PyObject *self, PyObject
*** 2852,2857
--- 2854,2862
plan-values = PLy_malloc(sizeof(Datum) * nargs);
plan-args = PLy_malloc(sizeof(PLyTypeInfo) * nargs);
+ BeginInternalSubTransaction(NULL);
+ MemoryContextSwitchTo(oldcontext);
+
PG_TRY();
{
int i;
*** PLy_spi_prepare(PyObject *self, PyObject
*** 2931,2949
if (plan-plan == NULL)
elog(ERROR, SPI_saveplan failed: %s,
SPI_result_code_string(SPI_result));
}
PG_CATCH();
{
ErrorData *edata;
- MemoryContextSwitchTo(oldcontext);
- edata = CopyErrorData();
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_exception_set(PLy_exc_spi_error, edata-message);
return NULL;
}
--- 2936,2978
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;
