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.






--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to