Re: pl/python custom exceptions for SPI

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
>