Re: pl/python SPI in subtransactions - Mailing list pgsql-hackers

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
>
>
>
>