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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
