Re: [PATCH] PL/Python: Add spidata to all spiexceptions

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 <kop(at)meme(dot)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