Hi, I don't feel particularly qualified to comment, but in the interest of (hopefully) helping with the patch review process I'm going to comment anyway.
(Having written this, I feel decidedly unqualified to critique so please keep this in mind when considering my comments.) On 10/31/2012 04:33:17 AM, Jan Urbański wrote: > You're right, I never thought of the possibility of user code > explicitly > throwing SPIError exceptions. > > The root issue is that PLy_elog will only set errcode if it finds the > "spidata" attribute, but I think passing error details through that > attribute is a kludge more than something more code should rely on. > > Here's an alternative patch that takes advantage of the alreadu > present > (and documented) "sqlstate" variable to set the error code when > handling > SPIError exceptions. It does seem to me that using sqlstate is the appropriate approach. If you're going to have user code throw the SPIError exception then shouldn't you allow them to also set detail and hints? Setting sqlstate seems a step in the right direction. I don't feel well enough qualified to say whether it goes far enough to be useful. I do note that pl/pgsql users are allowed to raise any 5 character error code regardless of whether it's listed in the documented set of error codes. This is a lot less useful if the code can't supply anything more than an error code. Extending the Python exception class to add attributes to SPIError for message, detail, and hint seems called for in the long run. I also wonder whether the if (sqlstate == NULL) test really belongs in the PLy_get_spi_sqlerrcode() code. Seems to me that different callers might need to do different things in this case, and that instead of PLy_get_spi_sqlerrcode you might instead want a function PLy_sqlstate_to_errcode(PyObject *sqlstate, int *sqlerrcode) and do the if (sqlstate == NULL) test (and the surrounding infrastructure) in the calling code. Regards, Karl <k...@meme.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein P.S. For reasons I don't understand I can't seem to download your patch directly from the mailing list archive at: http://archives.postgresql.org/pgsql-hackers/2012-10/msg01590.php -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers