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

PL/Python maps Python SPIError exceptions with 'spidata' attribute into SQL
errors. PL/Python also creates classes in plpy.spiexceptions for all known
errors but does not initialize their spidata, so when a PL/Python function
raises such an exception it is not recognized properly and is always
reported as an internal error.

This allows PL/Python code to raise exceptions that PL/pgSQL can catch and
which are correctly reported in logs instead of always showing up as XX000.
---
src/pl/plpython/expected/plpython_error.out | 12 ++++++++++++
src/pl/plpython/expected/plpython_error_0.out | 12 ++++++++++++
src/pl/plpython/plpy_plpymodule.c | 9 +++++++++
src/pl/plpython/sql/plpython_error.sql | 14 ++++++++++++++
4 files changed, 47 insertions(+)

diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index e1ec9c2..c1c36d9 100644
--- a/src/pl/plpython/expected/plpython_error.out
+++ b/src/pl/plpython/expected/plpython_error.out
@@ -400,3 +400,15 @@ CONTEXT: Traceback (most recent call last):
PL/Python function "manual_subxact_prepared", line 4, in <module>
plpy.execute(save)
PL/Python function "manual_subxact_prepared"
+/* raising plpy.spiexception.* from python code should preserve sqlstate
+ */
+CREATE FUNCTION plpy_raise_spiexception() RETURNS void AS $$
+raise plpy.spiexceptions.DivisionByZero()
+$$ LANGUAGE plpythonu;
+DO $$
+BEGIN
+ SELECT plpy_raise_spiexception();
+EXCEPTION WHEN division_by_zero THEN
+ -- NOOP
+END
+$$ LANGUAGE plpgsql;
diff --git a/src/pl/plpython/expected/plpython_error_0.out b/src/pl/plpython/expected/plpython_error_0.out
index 6f74a50..61d95e3 100644
--- a/src/pl/plpython/expected/plpython_error_0.out
+++ b/src/pl/plpython/expected/plpython_error_0.out
@@ -400,3 +400,15 @@ CONTEXT: Traceback (most recent call last):
PL/Python function "manual_subxact_prepared", line 4, in <module>
plpy.execute(save)
PL/Python function "manual_subxact_prepared"
+/* raising plpy.spiexception.* from python code should preserve sqlstate
+ */
+CREATE FUNCTION plpy_raise_spiexception() RETURNS void AS $$
+raise plpy.spiexceptions.DivisionByZero()
+$$ LANGUAGE plpythonu;
+DO $$
+BEGIN
+ SELECT plpy_raise_spiexception();
+EXCEPTION WHEN division_by_zero THEN
+ -- NOOP
+END
+$$ LANGUAGE plpgsql;
diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
index 37ea2a4..4213e83 100644
--- a/src/pl/plpython/plpy_plpymodule.c
+++ b/src/pl/plpython/plpy_plpymodule.c
@@ -247,6 +247,7 @@ PLy_generate_spi_exceptions(PyObject *mod, PyObject *base)
PyObject *exc;
PLyExceptionEntry *entry;
PyObject *sqlstate;
+ PyObject *spidata;
PyObject *dict = PyDict_New();

if (dict == NULL)
@@ -258,6 +259,14 @@ PLy_generate_spi_exceptions(PyObject *mod, PyObject *base)

PyDict_SetItemString(dict, "sqlstate", sqlstate);
Py_DECREF(sqlstate);
+
+ spidata = Py_BuildValue("izzzi", exception_map[i].sqlstate,
+ NULL, NULL, NULL, 0);
+ if (spidata == NULL)
+ PLy_elog(ERROR, "could not generate SPI exceptions");
+ PyDict_SetItemString(dict, "spidata", spidata);
+ Py_DECREF(spidata);
+
exc = PyErr_NewException(exception_map[i].name, base, dict);
PyModule_AddObject(mod, exception_map[i].classname, exc);
entry = hash_search(PLy_spi_exceptions, &exception_map[i].sqlstate,
diff --git a/src/pl/plpython/sql/plpython_error.sql b/src/pl/plpython/sql/plpython_error.sql
index 502bbec..ec93144 100644
--- a/src/pl/plpython/sql/plpython_error.sql
+++ b/src/pl/plpython/sql/plpython_error.sql
@@ -298,3 +298,17 @@ plpy.execute(rollback)
$$ LANGUAGE plpythonu;

SELECT manual_subxact_prepared();
+
+/* raising plpy.spiexception.* from python code should preserve sqlstate
+ */
+CREATE FUNCTION plpy_raise_spiexception() RETURNS void AS $$
+raise plpy.spiexceptions.DivisionByZero()
+$$ LANGUAGE plpythonu;
+
+DO $$
+BEGIN
+ SELECT plpy_raise_spiexception();
+EXCEPTION WHEN division_by_zero THEN
+ -- NOOP
+END
+$$ LANGUAGE plpgsql;
--
1.7.12.1

On 30/10/12 22:06, Oskari Saarenmaa wrote:
> PL/Python maps Python SPIError exceptions with 'spidata' attribute into SQL
> errors. PL/Python also creates classes in plpy.spiexceptions for all known
> errors but does not initialize their spidata, so when a PL/Python function
> raises such an exception it is not recognized properly and is always
> reported as an internal error.

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 already present
(and documented) "sqlstate" variable to set the error code when handling
SPIError exceptions.

I also used your test case and added another one, just in case.

Thanks,
Jan

On Wed, Oct 31, 2012 at 5:33 AM, Jan Urbański <wulczer(at)wulczer(dot)org> wrote:
> On 30/10/12 22:06, Oskari Saarenmaa wrote:
>>
>> PL/Python maps Python SPIError exceptions with 'spidata' attribute into
>> SQL
>> errors. PL/Python also creates classes in plpy.spiexceptions for all
>> known
>> errors but does not initialize their spidata, so when a PL/Python function
>> raises such an exception it is not recognized properly and is always
>> reported as an internal error.
>
> 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 already present (and
> documented) "sqlstate" variable to set the error code when handling SPIError
> exceptions.
>
> I also used your test case and added another one, just in case.

You should probably add this to the next CF so we don't forget about it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 05/11/12 18:35, Robert Haas wrote:
> On Wed, Oct 31, 2012 at 5:33 AM, Jan Urbański<wulczer(at)wulczer(dot)org> wrote:
>> On 30/10/12 22:06, Oskari Saarenmaa wrote:
>>>
>>> PL/Python maps Python SPIError exceptions with 'spidata' attribute into
>>> SQL
>>> errors.
>>
>> Here's an alternative patch that takes advantage of the already present (and
>> documented) "sqlstate" variable to set the error code when handling SPIError
>> exceptions.
>>
>> I also used your test case and added another one, just in case.
>
> You should probably add this to the next CF so we don't forget about it.

I will, as soon as I recover my community account.

Cheers,
Jan

On 05/11/12 19:07, Jan Urbański wrote:
> On 05/11/12 18:35, Robert Haas wrote:
>>
>> You should probably add this to the next CF so we don't forget about it.
>
> I will, as soon as I recover my community account.

Added as https://commitfest.postgresql.org/action/patch_view?id=971

J

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

On 12/09/2012 10:33:59 PM, Karl O. Pinc wrote:
> 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.

I've gone ahead and signed up to review this patch.

I can confirm that it compiles against head and
the tests pass. I started up a server
and tried it and it works for me, trapping
the exception and executing the exception code.

So, looks good, as far as it goes.
I await your response to my previous message
in this thread before sending it on a
a committer. There were 2 outstanding
issue raised:

Is this useful enough/proceeding in the right
direction to commit now?

Should some of the logic be moved out of a
subroutine and into the calling code?

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

Though not the original patch autor, I did modify and submit it to the
CF app, so I'll reply here :)

On 10/12/12 19:20, Karl O. Pinc wrote:
> On 12/09/2012 10:33:59 PM, Karl O. Pinc wrote:

> I've gone ahead and signed up to review this patch.

Thanks!

> There were 2 outstanding issue raised:
>
> Is this useful enough/proceeding in the right direction to commit
> now?

I believe the process would be to mark it as "Ready for Committer" if
you feel like it's ready and a then a committer would make the final call.

> Should some of the logic be moved out of a subroutine and into the
> calling code?

I think I structured the PLy_get_spi_sqlerrcode to accept the same kind
of arguments as PLy_get_spi_error_data, which means a SPIException
object and a pointer to whatever values it can fill in.

That said, I haven't really thought about that too much, so I'm
perfectly fine with moving that bit of logic to the caller.

Cheers,
Jan

Hi Jan and Oskari,

On 12/12/2012 11:36:54 AM, Jan Urbański wrote:
> On 10/12/12 19:20, Karl O. Pinc wrote:
> > On 12/09/2012 10:33:59 PM, Karl O. Pinc wrote:

> > There were 2 outstanding issue raised:
> >
> > Is this useful enough/proceeding in the right direction to commit
> > now?
>
> I believe the process would be to mark it as "Ready for Committer" if
> you feel like it's ready and a then a committer would make the final
> call.

It looks acceptable to me. My concern is that there's nothing
introduced here that precludes attaching additional attributes
to the exception object to carry message, detail, and hint.
I don't see a problem, and can't see how there could be a problem
but also feel like a novice. I was looking for some assurance from
you that there's no concern here, but am confident enough regardless
to pass this aspect through to a committer for final review.

>
> > Should some of the logic be moved out of a subroutine and into the
> > calling code?
>
> I think I structured the PLy_get_spi_sqlerrcode to accept the same
> kind
> of arguments as PLy_get_spi_error_data, which means a SPIException
> object and a pointer to whatever values it can fill in.
>
> That said, I haven't really thought about that too much, so I'm
> perfectly fine with moving that bit of logic to the caller.

I can see arguments to be made for both sides. I'm asking that you
think it through to the extent you deem necessary and make
changes or not. At that point it should be ready to send
to a committer. (I'll re-test first, if you make any changes.)

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On 12/12/12 20:21, Karl O. Pinc wrote:
>>> There were 2 outstanding issue raised:
>>>
>>> Is this useful enough/proceeding in the right direction to commit
>>> now?
>>
>>> Should some of the logic be moved out of a subroutine and into the
>>> calling code?
>>

> I can see arguments to be made for both sides. I'm asking that you
> think it through to the extent you deem necessary and make
> changes or not. At that point it should be ready to send
> to a committer. (I'll re-test first, if you make any changes.)

Oh my, I have dropped the ball on this one in the worst manner possible.
Sorry!

I actually still prefer to keep the signatures of PLy_get_spi_sqlerrcode
and PLy_get_spi_error_data similar, so I'd opt for keeping the patch
as-is :)

Thanks,
Jan

On 01/09/2013 01:08:39 PM, Jan Urbański wrote:

> > I can see arguments to be made for both sides. I'm asking that you
> > think it through to the extent you deem necessary and make
> > changes or not. At that point it should be ready to send
> > to a committer. (I'll re-test first, if you make any changes.)
>
> Oh my, I have dropped the ball on this one in the worst manner
> possible.
> Sorry!

My fault too. I've been thinking I should write to remind you.

>
> I actually still prefer to keep the signatures of
> PLy_get_spi_sqlerrcode
> and PLy_get_spi_error_data similar, so I'd opt for keeping the patch
> as-is :)

I will send it on to a committer.

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein

On 09.01.2013 21:29, Karl O. Pinc wrote:
> On 01/09/2013 01:08:39 PM, Jan Urbański wrote:
>> I actually still prefer to keep the signatures of
>> PLy_get_spi_sqlerrcode
>> and PLy_get_spi_error_data similar, so I'd opt for keeping the patch
>> as-is :)
>
> I will send it on to a committer.

Looks good to me. Committed, thanks!

- Heikki