Horton

Thread

  1. pl/python explicit subtransactions

    Jan Urbański <wulczer@wulczer.org> — 2010-12-23T14:32:02Z

    Here's a patch implementing explicitly starting subtransactions mentioned in
http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
an incremental patch on top of the spi-in-subxacts patch sent eariler.

Git branch for this patch:
https://github.com/wulczer/postgres/tree/explicit-subxacts.

The idea has been proposed in
http://archives.postgresql.org/pgsql-hackers/2010-11/msg00122.php

This patch provides a subtransaction context manager, in the vein of
http://www.python.org/dev/peps/pep-0343/.

When inside an explicit subtransaction, SPI calls do not start another
one, so you pay the subxact start overhead only once, and you get atomic
behaviour.

For instance this:

with plpy.subxact():
    plpy.execute("insert into t values (1)")
    plpy.execute("insert into t values (2)")
    plpy.execute("ooops")

will not insert any rows into t. Just so you realise it, it *will* raise
the exception from the last execute, if you want to continue execution
you need to put your with block in a try/catch. If the code starts a
subtransaction but fails to close it, PL/Python will forcibly roll it
back to leave the backend in a clean state.

The patch lacks user-facing documentation, I'll add that later if it
gets accepted. For more usage examples refer to the unit tests that the
patch adds.

Cheers,
Jan

  2. Re: pl/python explicit subtransactions

    Jan Urbański <wulczer@wulczer.org> — 2011-01-27T22:11:04Z

    On 23/12/10 15:32, Jan Urbański wrote:
> Here's a patch implementing explicitly starting subtransactions mentioned in
> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
> an incremental patch on top of the spi-in-subxacts patch sent eariler.

Updated to the spi-in-subxacts version sent earlier.

  3. Re: pl/python explicit subtransactions

    Steve Singer <ssinger@ca.afilias.info> — 2011-02-02T13:16:21Z

    On 11-01-27 05:11 PM, Jan Urbański wrote:
> On 23/12/10 15:32, Jan Urbański wrote:
>> Here's a patch implementing explicitly starting subtransactions mentioned in
>> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
>> an incremental patch on top of the spi-in-subxacts patch sent eariler.
>
> Updated to the spi-in-subxacts version sent earlier.
>
>
>


Submission Review
-----------------

The patch applies against master.
Test updates are included.

The patch doesn't included any documentation updates.  The author did 
mention that he'll do these if it looks like the patch is going to be 
accepted.

The plpython_subxact regression test you addded is failing on both 
python3 and 2.4 for me.  It seems to be creating functions with the same 
name twice and the second time is failing with "ERROR: function ....." 
already exists.  I think this is just an issue with your expect files.

Usability Review
---------------

The patch implements a python context manager that allows plpython 
programs to control subtransactions with the python 'with' syntax.
The patch implements what it describes.  Using the subtransaction 
manager seems consistent with other examples of Python context managers. 
  This feature seems useful for pl/python developers.

The 'with' syntax was only officially added with python 2.6.  I have 
confirmed that the patch does not break plpython going as far back as 
2.5 and 2.4.  I have no reason to think that earlier versions will be 
broken either, I just didn't test anything earlier than 2.4.

I think this feature is useful for developing more complicated functions 
in pl/python and we don't have an existing way of managing savepoints 
from pl/python.  The context manager approach seems consistent with how 
recent python versions deal with this type of thing in other areas.



Feature Test
------------
No issues found.


Code Review
------------


PLy_abort_open_subtransactions(...) line 1215:

	ereport(WARNING,
		(errmsg("Forcibly aborting a subtransaction "
		"that has not been exited")));

"Forcibly" should be "forcibly" (lower case)

Similarly in PLy_subxact_enter and PLy_subxact_exit a few
PLy_exception_set calls start with an upper case character when I think 
we want it to be lower case.


Other than that I don't see any issues.  I am marking this as waiting 
for author since the documentation is still outstanding.

  4. Re: pl/python explicit subtransactions

    Peter Eisentraut <peter_e@gmx.net> — 2011-02-02T21:25:02Z

    On tor, 2010-12-23 at 15:32 +0100, Jan Urbański wrote:
> with plpy.subxact():
>     plpy.execute("insert into t values (1)")
>     plpy.execute("insert into t values (2)")
>     plpy.execute("ooops")

Looks pretty cool, but maybe s/subxact/subtransaction/.

  5. Re: pl/python explicit subtransactions

    Jan Urbański <wulczer@wulczer.org> — 2011-02-06T09:52:56Z

    On 02/02/11 14:16, Steve Singer wrote:
> On 11-01-27 05:11 PM, Jan Urbański wrote:
>> On 23/12/10 15:32, Jan Urbański wrote:
>>> Here's a patch implementing explicitly starting subtransactions
>>> mentioned in
>>> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
>>> an incremental patch on top of the spi-in-subxacts patch sent eariler.
>>
>> Updated to the spi-in-subxacts version sent earlier.
>>
> [review]
> 
> Other than that I don't see any issues.  I am marking this as waiting
> for author since the documentation is still outstanding.

Thanks Steve, I'm writing docs and changing error messages to lowercase
as you suggested and will send an updated patch today.

Jan

PS: Oh, I just saw Peter's suggestion to rename the function from
plpy.subxact() to plpy.subtransaction(). Fine with me, will do that too.

J

  6. Re: pl/python explicit subtransactions

    Jan Urbański <wulczer@wulczer.org> — 2011-02-06T16:40:25Z

    On 02/02/11 14:16, Steve Singer wrote:
> On 11-01-27 05:11 PM, Jan Urbański wrote:
>> On 23/12/10 15:32, Jan Urbański wrote:
>>> Here's a patch implementing explicitly starting subtransactions
>>> mentioned in
>>> http://archives.postgresql.org/pgsql-hackers/2010-12/msg01991.php. It's
>>> an incremental patch on top of the spi-in-subxacts patch sent eariler.
>>
>> Updated to the spi-in-subxacts version sent earlier.
>>
> 
> Submission Review
> -----------------
> 
> The patch applies against master.
> Test updates are included.
> 
> The patch doesn't included any documentation updates.  The author did
> mention that he'll do these if it looks like the patch is going to be
> accepted.

PFA an updated patch with documentation.

> The plpython_subxact regression test you addded is failing on both
> python3 and 2.4 for me.  It seems to be creating functions with the same
> name twice and the second time is failing with "ERROR: function ....."
> already exists.  I think this is just an issue with your expect files.

The expect files for older Pythons were broken by the change to include
HINT and DETAIL messages when errors are reported from Python. I fixed
them and now the regression tests are passing for me on Python 2.4, 2.6
and 3.1.

> Code Review
> ------------
> 
> 
> PLy_abort_open_subtransactions(...) line 1215:
> 
>     ereport(WARNING,
>         (errmsg("Forcibly aborting a subtransaction "
>         "that has not been exited")));
> 
> "Forcibly" should be "forcibly" (lower case)
> 
> Similarly in PLy_subxact_enter and PLy_subxact_exit a few
> PLy_exception_set calls start with an upper case character when I think
> we want it to be lower case.

Yeah, changed them.

Thanks,
Jan

  7. Re: pl/python explicit subtransactions

    Steve Singer <ssinger_pg@sympatico.ca> — 2011-02-08T05:32:38Z

    On 11-02-06 11:40 AM, Jan Urbański wrote:

> PFA an updated patch with documentation.

> Yeah, changed them.

Those changes look fine.  The tests now pass.

I've attached a new version of the patch that fixes a few typos/wording 
issues I saw in the documentation.  I also changed the link to the 
python reference manual section on context managers. I think it is 
better to link to that versus the original PEP.

The documentation could probably still use more word-smithing but that 
can happen later.  I'm marking this as ready for a committer.



>
> Thanks,
> Jan

  8. Re: pl/python explicit subtransactions

    Peter Eisentraut <peter_e@gmx.net> — 2011-02-09T22:22:04Z

    On tis, 2011-02-08 at 00:32 -0500, Steve Singer wrote:
> On 11-02-06 11:40 AM, Jan Urbański wrote:
> 
> > PFA an updated patch with documentation.
> 
> > Yeah, changed them.
> 
> Those changes look fine.  The tests now pass.
> 
> I've attached a new version of the patch that fixes a few typos/wording 
> issues I saw in the documentation.  I also changed the link to the 
> python reference manual section on context managers. I think it is 
> better to link to that versus the original PEP.
> 
> The documentation could probably still use more word-smithing but that 
> can happen later.  I'm marking this as ready for a committer.

Is it necessarily a good idea that an explicit subtransaction disables
the implicit sub-subtransactions?  It might be conceivable that you'd
still want to do some try/catch within explicit subtransactions.

  9. Re: pl/python explicit subtransactions

    Steve Singer <ssinger_pg@sympatico.ca> — 2011-02-10T00:26:31Z

    On 11-02-09 05:22 PM, Peter Eisentraut wrote:
> On tis, 2011-02-08 at 00:32 -0500, Steve Singer wrote:
>> On 11-02-06 11:40 AM, Jan Urbański wrote:
>>
>>> PFA an updated patch with documentation.
>>> Yeah, changed them.
>> Those changes look fine.  The tests now pass.
>>
>> I've attached a new version of the patch that fixes a few typos/wording
>> issues I saw in the documentation.  I also changed the link to the
>> python reference manual section on context managers. I think it is
>> better to link to that versus the original PEP.
>>
>> The documentation could probably still use more word-smithing but that
>> can happen later.  I'm marking this as ready for a committer.
> Is it necessarily a good idea that an explicit subtransaction disables
> the implicit sub-subtransactions?  It might be conceivable that you'd
> still want to do some try/catch within explicit subtransactions.
>
>

I had tested nested subtransactions but not a normal try/catch within a 
subtransaction.  That sounds reasonable to allow.

Unfortunately it leads to:


test=# create table foo(a int4 primary key);
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index 
"foo_pkey" for table "foo"
CREATE TABLE
test=# DO $$
test$# try:
test$#   with plpy.subtransaction():
test$#     plpy.execute("insert into foo values(1)")
test$#     try:
test$#       plpy.execute("insert into foo values(1)")
test$#     except:
test$#       plpy.notice('inside exception')
test$# except plpy.SPIError:
test$#   f=0
test$# $$ language plpythonu;
TRAP: FailedAssertion("!(afterTriggers->query_depth == 
afterTriggers->depth_stack[my_level])", File: "trigger.c", Line: 3846)
NOTICE:  inside exception
CONTEXT:  PL/Python anonymous code block
server closed the connection unexpectedly
     This probably means the server terminated abnormally
     before or while processing the request.

  10. Re: pl/python explicit subtransactions

    Jan Urbański <wulczer@wulczer.org> — 2011-02-10T10:20:06Z

    On 10/02/11 01:26, Steve Singer wrote:
> On 11-02-09 05:22 PM, Peter Eisentraut wrote:
>> On tis, 2011-02-08 at 00:32 -0500, Steve Singer wrote:
>> Is it necessarily a good idea that an explicit subtransaction disables
>> the implicit sub-subtransactions?  It might be conceivable that you'd
>> still want to do some try/catch within explicit subtransactions.
>>
> 
> I had tested nested subtransactions but not a normal try/catch within a
> subtransaction.  That sounds reasonable to allow.
> 
> Unfortunately it leads to:
> 
> [crash]

D'oh, I was thinking about whether it's safe to skip the internal
subxact if you're in an implicit one and somehow I always convinced
myself that since you eventually close the explicit one, it is.

Obviously my testing wasn't enough :( Attaching an updated patch with
improved docs incorporating Steve's fixes, and fixes & tests for not
statring the implicit subxact. That actually makes the patch a bit
smaller ;) OTOH I had to remove the section from the docs that claimed
performance improvement due to only starting the explicit subxact...

Cheers,
Jan

  11. Re: pl/python explicit subtransactions

    Steve Singer <ssinger_pg@sympatico.ca> — 2011-02-11T16:22:17Z

    On 11-02-10 05:20 AM, Jan Urbański wrote:
>
> D'oh, I was thinking about whether it's safe to skip the internal
> subxact if you're in an implicit one and somehow I always convinced
> myself that since you eventually close the explicit one, it is.
>
> Obviously my testing wasn't enough :( Attaching an updated patch with
> improved docs incorporating Steve's fixes, and fixes&  tests for not
> statring the implicit subxact. That actually makes the patch a bit
> smaller ;) OTOH I had to remove the section from the docs that claimed
> performance improvement due to only starting the explicit subxact...
>

This version of the patch looks fine to me and seems to work as expected.


> Cheers,
> Jan
>
>
>

  12. Re: pl/python explicit subtransactions

    Jan Urbański <wulczer@wulczer.org> — 2011-02-12T10:58:39Z

    On 11/02/11 17:22, Steve Singer wrote:
> On 11-02-10 05:20 AM, Jan Urbański wrote:
>>
>> D'oh, I was thinking about whether it's safe to skip the internal
>> subxact if you're in an implicit one and somehow I always convinced
>> myself that since you eventually close the explicit one, it is.
>>
>> Obviously my testing wasn't enough :( Attaching an updated patch with
>> improved docs incorporating Steve's fixes, and fixes & tests for not
>> statring the implicit subxact. That actually makes the patch a bit
>> smaller ;) OTOH I had to remove the section from the docs that claimed
>> performance improvement due to only starting the explicit subxact...
>>
> 
> This version of the patch looks fine to me and seems to work as expected.

Thanks,

attached is a version merged with master.

Jan

  13. Re: pl/python explicit subtransactions

    Robert Haas <robertmhaas@gmail.com> — 2011-02-26T05:12:36Z

    On Wed, Feb 9, 2011 at 5:22 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On tis, 2011-02-08 at 00:32 -0500, Steve Singer wrote:
>> On 11-02-06 11:40 AM, Jan Urbański wrote:
>>
>> > PFA an updated patch with documentation.
>>
>> > Yeah, changed them.
>>
>> Those changes look fine.  The tests now pass.
>>
>> I've attached a new version of the patch that fixes a few typos/wording
>> issues I saw in the documentation.  I also changed the link to the
>> python reference manual section on context managers. I think it is
>> better to link to that versus the original PEP.
>>
>> The documentation could probably still use more word-smithing but that
>> can happen later.  I'm marking this as ready for a committer.
>
> Is it necessarily a good idea that an explicit subtransaction disables
> the implicit sub-subtransactions?  It might be conceivable that you'd
> still want to do some try/catch within explicit subtransactions.

Is this still an open question, or what is the remaining issue that
needs to be addressed with regards to this patch?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  14. Re: pl/python explicit subtransactions

    Jan Urbański <wulczer@wulczer.org> — 2011-02-26T08:38:31Z

    ----- Original message -----
> On Wed, Feb 9, 2011 at 5:22 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> > On tis, 2011-02-08 at 00:32 -0500, Steve Singer wrote:
> > > The documentation could probably still use more word-smithing but
> > > that can happen later.  I'm marking this as ready for a committer.
> > 
> > Is it necessarily a good idea that an explicit subtransaction disables
> > the implicit sub-subtransactions?  It might be conceivable that you'd
> > still want to do some try/catch within explicit subtransactions.
> 
> Is this still an open question, or what is the remaining issue that
> needs to be addressed with regards to this patch?

The docs are included in the latest patch, and it turned out that disabling implicit subxacts inside explicit subxacts is not a good idea, so it's been fixed in the last patch. There are no unresolved issues AFAICT.

Jan

  15. Re: pl/python explicit subtransactions

    Robert Haas <robertmhaas@gmail.com> — 2011-02-26T14:49:08Z

    2011/2/26 Jan Urbański <wulczer@wulczer.org>:
> The docs are included in the latest patch, and it turned out that disabling implicit subxacts inside explicit subxacts is not a good idea, so it's been fixed in the last patch. There are no unresolved issues AFAICT.

OK.  Peter, are you planning to commit this?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  16. Re: pl/python explicit subtransactions

    Peter Eisentraut <peter_e@gmx.net> — 2011-02-26T15:08:46Z

    On lör, 2011-02-26 at 09:49 -0500, Robert Haas wrote:
> OK.  Peter, are you planning to commit this?

Yes.

  17. Re: pl/python explicit subtransactions

    Peter Eisentraut <peter_e@gmx.net> — 2011-02-27T19:20:06Z

    Committed.