Hello, Here's a detailed review:
1. in PLy_spi_error__init__ you need to check kw for NULL before doing PyDict_Size(kw) otherwise for plpy.SPIError() you get Bad internal call because PyDict_Size expects a real dictionary not NULL 2. a test with just plpy.SPIError() is still missing, this would have caught 1. 3. the tests have "possibility to set all accessable fields in custom exception" above a test that doesn't set all fields, it's confusing (and accessible is spelled wrong) 4. in the docs, "The constructor of SPIError exception (class) supports keyword parameters." sounds better as "An SPIError instance can be constructed using keyword parameters." 5. there is conceptual code duplication between PLy_spi_exception_set in plpy_spi.c, since that code also constructs an SPIError but from C and with more info available (edata->internalquery and edata->internalpos). But making a tuple and assigning it to spidata is in both. Not sure how this can be improved. 6. __init__ can use METH_VARARGS | METH_KEYWORDS in both Python2 and 3, no need for the #if 7. "could not create dictionary for SPI exception" would be more precise as "could not create dictionary for SPIError" right? 8. in PLy_add_methods_to_dictionary I would avoid import since it makes one think of importing modules; maybe "could not create function wrapper for \"%s\"", "could not create method wrapper for \"%s\"" 9. also in PLy_add_methods_to_dictionary "could public method \"%s\" in dictionary" is not proper English and is missing not, maybe "could not add method \"%s\" to class dictionary"? 10. in PLy_add_methods_to_dictionary if PyCFunction_New succeeds but PyMethod_New fails, func will leak 11. it would be nice to have a test for the invalid SQLSTATE code part 12. similar to 10, in PLy_spi_error__init__ taking the "invalid SQLSTATE" branch leaks exc_args, in general early returns via PLy_elog will leave the intermediate Python objects leaking Will mark the patch as Waiting for author. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
