Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
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 Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
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 Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
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 Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [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 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
Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
On 05/11/12 18:35, Robert Haas wrote: On Wed, Oct 31, 2012 at 5:33 AM, Jan Urbański 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
On Wed, Oct 31, 2012 at 5:33 AM, Jan Urbański 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] PL/Python: Add spidata to all spiexceptions
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 >From 633fe3cf5872f57fcc33c5462048c0cfea81d907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Urba=C5=84ski?= Date: Wed, 31 Oct 2012 10:27:59 +0100 Subject: [PATCH] Set SQL error code for SPI errors raised from userland Python code. If a PL/Python function was terminated by a SPIError raised from user code, the Postgres-level error code was always set to internal_error. This makes it pay attention to the exception's sqlstate attribute and set the error code accordingly. --- src/pl/plpython/expected/plpython_error.out | 26 src/pl/plpython/expected/plpython_error_0.out | 26 src/pl/plpython/plpy_elog.c | 40 + src/pl/plpython/sql/plpython_error.sql| 30 +++ 4 files changed, 117 insertions(+), 5 deletions(-) diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out index e1ec9c2..be2ec97 100644 --- a/src/pl/plpython/expected/plpython_error.out +++ b/src/pl/plpython/expected/plpython_error.out @@ -400,3 +400,29 @@ CONTEXT: Traceback (most recent call last): PL/Python function "manual_subxact_prepared", line 4, in 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; +/* setting a custom sqlstate should be handled + */ +CREATE FUNCTION plpy_raise_spiexception_override() RETURNS void AS $$ +exc = plpy.spiexceptions.DivisionByZero() +exc.sqlstate = 'SILLY' +raise exc +$$ LANGUAGE plpythonu; +DO $$ +BEGIN + SELECT plpy_raise_spiexception_override(); +EXCEPTION WHEN SQLSTATE 'SILLY' 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..39c63c5 100644 --- a/src/pl/plpython/expected/plpython_error_0.out +++ b/src/pl/plpython/expected/plpython_error_0.out @@ -400,3 +400,29 @@ CONTEXT: Traceback (most recent call last): PL/Python function "manual_subxact_prepared", line 4, in 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; +/* setting a custom sqlstate should be handled + */ +CREATE FUNCTION plpy_raise_spiexception_override() RETURNS void AS $$ +exc = plpy.spiexceptions.DivisionByZero() +exc.sqlstate = 'SILLY' +raise exc +$$ LANGUAGE plpythonu; +DO $$ +BEGIN + SELECT plpy_raise_spiexception_override(); +EXCEPTION WHEN SQLSTATE 'SILLY' THEN + -- NOOP +END +$$ LANGUAGE plpgsql; diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c index c375ac0..e3044e7 100644 --- a/src/pl/plpython/plpy_elog.c +++ b/src/pl/plpython/plpy_elog.c @@ -336,6 +336,29 @@ PLy_traceback(char **xmsg, char **tbmsg, int *tb_depth) Py_DECREF(e); } + +static void +PLy_get_spi_sqlerrcode(PyObject *exc, int *sqlerrcode) +{ + PyObject *sqlstate; + char *buffer; + + sqlstate = PyObject_GetAttrString(exc, "sqlstate"); + if (sqlstate == NULL) + return; + + buffer = PyString_AsString(sqlstate); + if (strlen(buffer) == 5 && + strspn(buffer, "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ") == 5) + { + *sqlerrcode = MAKE_SQLSTATE(buffer[0], buffer[1], buffer[2], + buffer[3], buffer[4]); + } + + Py_DECREF(sqlstate); +} + + /* * Extract the error data from a SPIError */ @@ -345,13 +368,20 @@ PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail, char **hin PyObject *spidata = NULL; spidata = PyObject_GetAttrString(exc, "spida
[HACKERS] [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 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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers