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
>