Re: pl/python explicit subtransactions

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.