On 11-02-10 03:13 PM, Jan Urbański wrote:
On 10/02/11 20:24, Peter Eisentraut wrote:
Here is the rest of my review.
Submission Review
-------------------
Patch applies cleanly.
Documentation is still outstanding but Jan has promised it soon.
Usability Review
-------------------
We don't have this for plpython, that we have a similar idea with
plpgsql. I think this feature is useful and worth having.
The CamelCase naming of the exceptions is consistent with how the
built-in python exceptions are named (camel case).
Feature Test
---------------
I did basic testing of the feature (catching a few exception types
thrown from both direct SQL and prepared statements) and the feature
worked as expected.
Performance Impact
--------------------
The impact of mapping error codes to exception types shouldn't come into
play unless an SPI error is returned and with the hash it should still
be minimal.
Code Review
-------------
Ideally char * members of ExceptionMap would be const, but since many
versions of python take a non-const value to PyErr_NewException that
won't work :(
After you search the for an exception in the hash you have:
/* We really should find it, but just in case have a fallback */
Assert(entry != NULL);
exc = entry ? entry->exc : PLy_exc_spi_error;
I'm not sure the assert is needed here. Just falling back to the
exception type seems reasonable and more desirable than an assert if
showhow a new exception gets missed from the list. I don't feel that
strongly on this.
line 3575: PLy_elog(ERROR, "Failed to add the spiexceptions module");
"Failed" should be "failed"
Other than that the patch looks fine to me.
Updated again.
Why do the error messages print spiexceptions.SyntaxError instead of
plpy.spiexceptions.SyntaxError? Is this intentional or just the way it
comes out of Python?
That's how traceback.format_exception() works IIRC, which is what the
Python interpreter uses and what PL/Python mimicks in PLy_traceback.
Please add some documentation. Not a list of all exceptions, but at
least a paragraph that various kinds of specific exceptions may be
generated, what package and module they are in, and how they relate.
Sure, Steve already asked for docs in another thread, and I'm writing them.
Jan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
