Re: pl/python custom exceptions for SPI

On 10/02/11 22:26, Steve Singer wrote:
> 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.

Thanks!

> 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 :(

Yeah, I got "discards qualifier" warnings when I tried to declare it as
const :(

> 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.

Maybe the comment doesn't explain this enough. My intention was that in
regular builds you have a fallback and if you're missing an entry in the
exceptions hash, you just get SPIError. But in assert-enabled builds you
get an error, so you can detect it and fix the exceptions hash.

> line 3575:     PLy_elog(ERROR, "Failed to add the spiexceptions module");
> "Failed" should be "failed"

Gah, I'll never learn. Will fix.

> Other than that the patch looks fine to me.

Thanks, I'll have the docs ready by today (and I should've have them by
yesterday :/).

Jan