Thread
-
[PATCH] PL/Python: Add spidata to all spiexceptions
Oskari Saarenmaa <os@ohmu.fi> — 2012-10-30T21:06:56Z
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 -
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Jan Urbański <wulczer@wulczer.org> — 2012-10-31T09:33:17Z
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
-
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Robert Haas <robertmhaas@gmail.com> — 2012-11-05T17:35:54Z
On Wed, Oct 31, 2012 at 5:33 AM, Jan Urbański <wulczer@wulczer.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
-
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Jan Urbański <wulczer@wulczer.org> — 2012-11-05T18:07:16Z
On 05/11/12 18:35, Robert Haas wrote: > On Wed, Oct 31, 2012 at 5:33 AM, Jan Urbański<wulczer@wulczer.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
-
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Jan Urbański <wulczer@wulczer.org> — 2012-11-05T18:45:52Z
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
-
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Karl O. Pinc <kop@meme.com> — 2012-12-10T04:33:59Z
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@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 -
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Karl O. Pinc <kop@meme.com> — 2012-12-10T18:20:08Z
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@meme.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein -
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Jan Urbański <wulczer@wulczer.org> — 2012-12-12T17:36:54Z
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
-
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Karl O. Pinc <kop@meme.com> — 2012-12-12T19:21:49Z
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@meme.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein -
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Jan Urbański <wulczer@wulczer.org> — 2013-01-09T19:08:39Z
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
-
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Karl O. Pinc <kop@meme.com> — 2013-01-09T19:29:37Z
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@meme.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein -
Re: [PATCH] PL/Python: Add spidata to all spiexceptions
Heikki Linnakangas <hlinnakangas@vmware.com> — 2013-01-28T07:48:59Z
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