Thread

1. pl/python SPI in subtransactions

   Jan Urbański <wulczer@wulczer.org> — 2010-12-23T13:45:28Z

   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
   

2. Re: pl/python SPI in subtransactions

   Steve Singer <ssinger_pg@sympatico.ca> — 2011-01-26T03:51:42Z

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

3. Re: pl/python SPI in subtransactions

   Jan Urbański <wulczer@wulczer.org> — 2011-01-27T21:33:01Z

   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
   

4. Re: pl/python SPI in subtransactions

   Steve Singer <ssinger_pg@sympatico.ca> — 2011-01-29T20:27:24Z

   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;
   
   
   
   

5. Re: pl/python SPI in subtransactions

   Jan Urbański <wulczer@wulczer.org> — 2011-01-29T20:39:24Z

   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
   

6. Re: pl/python SPI in subtransactions

   Steve Singer <ssinger_pg@sympatico.ca> — 2011-01-29T21:10:17Z

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

7. Re: pl/python SPI in subtransactions

   Jan Urbański <wulczer@wulczer.org> — 2011-01-29T21:13:13Z

   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
   
   

8. Re: pl/python SPI in subtransactions

   Jan Urbański <wulczer@wulczer.org> — 2011-01-30T23:03:25Z

   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
   

9. Re: pl/python SPI in subtransactions

   Jan Urbański <wulczer@wulczer.org> — 2011-01-30T23:10:53Z

   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
   
   

10. Re: pl/python SPI in subtransactions

    Peter Eisentraut <peter_e@gmx.net> — 2011-02-02T20:09:46Z

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