pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedPL/Python SQL error code pass-through
5+ messages / 3 participants
[nested] [flat]
* PL/Python SQL error code pass-through
@ 2011-11-23 16:24 Mika Eloranta <mel@ohmu.fi>
0 siblings, 1 reply; 5+ messages in thread
From: Mika Eloranta @ 2011-11-23 16:24 UTC (permalink / raw)
To: pgsql-hackers
Hi all,
Here's a little SQL snippet that exposes an apparent regression in the 9.1.x PL/Python behavior:
---clip---
# cat foo.sql
\set VERBOSITY 'verbose'
CREATE table bar (a INTEGER CONSTRAINT hello CHECK (a > 1));
CREATE OR REPLACE FUNCTION foo ()
RETURNS integer
AS $$
plpy.execute("INSERT INTO bar (a) VALUES (2)")
plpy.execute("INSERT INTO bar (a) VALUES (1)")
return 123
$$ LANGUAGE plpythonu;
SELECT * FROM foo();
---clip---
PostgreSQL 9.0 behavior:
---clip---
# psql < foo.sql
CREATE TABLE
CREATE FUNCTION
WARNING: 01000: PL/Python: plpy.SPIError: unrecognized error in PLy_spi_execute_query
CONTEXT: PL/Python function "foo"
LOCATION: PLy_elog, plpython.c:3532
ERROR: 23514: new row for relation "bar" violates check constraint "hello"
CONTEXT: SQL statement "INSERT INTO bar (a) VALUES (1)"
PL/Python function "foo"
LOCATION: ExecConstraints, execMain.c:1330
---clip---
Note the proper 23514 error code.
PostgreSQL 9.1.1 behavior:
---clip---
# psql < foo.sql
ERROR: 42P07: relation "bar" already exists
LOCATION: heap_create_with_catalog, heap.c:1011
CREATE FUNCTION
ERROR: XX000: spiexceptions.CheckViolation: new row for relation "bar" violates check constraint "hello"
CONTEXT: Traceback (most recent call last):
PL/Python function "foo", line 3, in <module>
plpy.execute("INSERT INTO bar (a) VALUES (1)")
PL/Python function "foo"
LOCATION: PLy_elog, plpython.c:4502
---clip---
In fact, all SQL error that occur within PL/Python seem to be returned with the "XX000" error code. This is a bit of a problem for client-side logic that detects e.g. constraint violations based on the SQL error code.
A small patch that includes passing thru the SQL error code is attached.
Test run with PostgreSQL 9.1.1 + patch:
---clip---
# psql < foo.sql
ERROR: 42P07: relation "bar" already exists
LOCATION: heap_create_with_catalog, heap.c:1011
CREATE FUNCTION
ERROR: 23514: spiexceptions.CheckViolation: new row for relation "bar" violates check constraint "hello"
CONTEXT: Traceback (most recent call last):
PL/Python function "foo", line 4, in <module>
plpy.execute("INSERT INTO bar (a) VALUES (1)")
PL/Python function "foo"
LOCATION: PLy_elog, plpython.c:4504
---clip---
Cheers!
- Mika
Attachments:
0001-PL-Python-SQL-error-code-pass-through.patch (application/octet-stream, 3.2K) download
From 82efddbac2efc74a932761b7cfbe55e31bc0011c Mon Sep 17 00:00:00 2001
From: Mika Eloranta <mel@ohmu.fi>
Date: Wed, 23 Nov 2011 18:04:37 +0200
Subject: [PATCH] PL/Python SQL error code pass-through
If a SQL error code is available, pass it though from PL/Python
instead of using "XX000" for all errors.
---
src/pl/plpython/plpython.c | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 93e8043..2071d8d 100644
--- a/src/pl/plpython/plpython.c
+++ b/src/pl/plpython/plpython.c
@@ -383,7 +383,7 @@ static char *PLy_procedure_name(PLyProcedure *);
static void
PLy_elog(int, const char *,...)
__attribute__((format(PG_PRINTF_ATTRIBUTE, 2, 3)));
-static void PLy_get_spi_error_data(PyObject *exc, char **detail, char **hint, char **query, int *position);
+static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail, char **hint, char **query, int *position);
static void PLy_traceback(char **, char **, int *);
static void *PLy_malloc(size_t);
@@ -4441,7 +4441,7 @@ PLy_spi_exception_set(PyObject *excclass, ErrorData *edata)
if (!spierror)
goto failure;
- spidata = Py_BuildValue("(zzzi)", edata->detail, edata->hint,
+ spidata = Py_BuildValue("(izzzi)", edata->sqlerrcode, edata->detail, edata->hint,
edata->internalquery, edata->internalpos);
if (!spidata)
goto failure;
@@ -4481,6 +4481,7 @@ PLy_elog(int elevel, const char *fmt,...)
*val,
*tb;
const char *primary = NULL;
+ int sqlerrcode = 0;
char *detail = NULL;
char *hint = NULL;
char *query = NULL;
@@ -4490,7 +4491,7 @@ PLy_elog(int elevel, const char *fmt,...)
if (exc != NULL)
{
if (PyErr_GivenExceptionMatches(val, PLy_exc_spi_error))
- PLy_get_spi_error_data(val, &detail, &hint, &query, &position);
+ PLy_get_spi_error_data(val, &sqlerrcode, &detail, &hint, &query, &position);
else if (PyErr_GivenExceptionMatches(val, PLy_exc_fatal))
elevel = FATAL;
}
@@ -4531,7 +4532,8 @@ PLy_elog(int elevel, const char *fmt,...)
PG_TRY();
{
ereport(elevel,
- (errmsg_internal("%s", primary ? primary : "no exception data"),
+ (errcode(sqlerrcode ? sqlerrcode : ERRCODE_INTERNAL_ERROR),
+ errmsg_internal("%s", primary ? primary : "no exception data"),
(detail) ? errdetail_internal("%s", detail) : 0,
(tb_depth > 0 && tbmsg) ? errcontext("%s", tbmsg) : 0,
(hint) ? errhint("%s", hint) : 0,
@@ -4562,7 +4564,7 @@ PLy_elog(int elevel, const char *fmt,...)
* Extract the error data from a SPIError
*/
static void
-PLy_get_spi_error_data(PyObject *exc, char **detail, char **hint, char **query, int *position)
+PLy_get_spi_error_data(PyObject *exc, int* sqlerrcode, char **detail, char **hint, char **query, int *position)
{
PyObject *spidata = NULL;
@@ -4570,7 +4572,7 @@ PLy_get_spi_error_data(PyObject *exc, char **detail, char **hint, char **query,
if (!spidata)
goto cleanup;
- if (!PyArg_ParseTuple(spidata, "zzzi", detail, hint, query, position))
+ if (!PyArg_ParseTuple(spidata, "izzzi", sqlerrcode, detail, hint, query, position))
goto cleanup;
cleanup:
--
1.7.6
=
* Re: PL/Python SQL error code pass-through
@ 2011-11-24 08:07 Jan Urbański <wulczer@wulczer.org>
parent: Mika Eloranta <mel@ohmu.fi>
0 siblings, 1 reply; 5+ messages in thread
From: Jan Urbański @ 2011-11-24 08:07 UTC (permalink / raw)
To: Mika Eloranta <mel@ohmu.fi>; +Cc: pgsql-hackers
On 23/11/11 17:24, Mika Eloranta wrote:
> Hi all,
>
> [PL/Python in 9.1 does not preserve SQLSTATE of errors]
Oops, you're right, it's a regression from 9.0 behaviour.
The fix looks good to me, I changed one place to indent with tabs
instead of spaces and added a regression test.
I think this should be backpatched to 9.1, no?
Thanks for the report and the patch!
Cheers,
Jan
Attachments:
plpython-error-passthrough-w-test.patch (text/x-diff, 7.0K) download
diff --git a/src/pl/plpython/expected/plpython_error.out b/src/pl/plpython/expected/plpython_error.out
index dbf19fd..bab07fb 100644
*** a/src/pl/plpython/expected/plpython_error.out
--- b/src/pl/plpython/expected/plpython_error.out
*************** CONTEXT: PL/Python function "specific_e
*** 351,356 ****
--- 351,378 ----
(1 row)
+ /* SPI errors in PL/Python functions should preserve the SQLSTATE value
+ */
+ CREATE FUNCTION python_unique_violation() RETURNS void AS $$
+ plpy.execute("insert into specific values (1)")
+ plpy.execute("insert into specific values (1)")
+ $$ LANGUAGE plpythonu;
+ CREATE FUNCTION catch_python_unique_violation() RETURNS text AS $$
+ begin
+ begin
+ perform python_unique_violation();
+ exception when unique_violation then
+ return 'ok';
+ end;
+ return 'not reached';
+ end;
+ $$ language plpgsql;
+ SELECT catch_python_unique_violation();
+ catch_python_unique_violation
+ -------------------------------
+ ok
+ (1 row)
+
/* manually starting subtransactions - a bad idea
*/
CREATE FUNCTION manual_subxact() RETURNS void AS $$
diff --git a/src/pl/plpython/expected/plpython_error_0.out b/src/pl/plpython/expected/plpython_error_0.out
index b2194ff..6cb2ed0 100644
*** a/src/pl/plpython/expected/plpython_error_0.out
--- b/src/pl/plpython/expected/plpython_error_0.out
*************** CONTEXT: PL/Python function "specific_e
*** 351,356 ****
--- 351,378 ----
(1 row)
+ /* SPI errors in PL/Python functions should preserve the SQLSTATE value
+ */
+ CREATE FUNCTION python_unique_violation() RETURNS void AS $$
+ plpy.execute("insert into specific values (1)")
+ plpy.execute("insert into specific values (1)")
+ $$ LANGUAGE plpythonu;
+ CREATE FUNCTION catch_python_unique_violation() RETURNS text AS $$
+ begin
+ begin
+ perform python_unique_violation();
+ exception when unique_violation then
+ return 'ok';
+ end;
+ return 'not reached';
+ end;
+ $$ language plpgsql;
+ SELECT catch_python_unique_violation();
+ catch_python_unique_violation
+ -------------------------------
+ ok
+ (1 row)
+
/* manually starting subtransactions - a bad idea
*/
CREATE FUNCTION manual_subxact() RETURNS void AS $$
diff --git a/src/pl/plpython/plpython.c b/src/pl/plpython/plpython.c
index 93e8043..afd5dfc 100644
*** a/src/pl/plpython/plpython.c
--- b/src/pl/plpython/plpython.c
*************** static char *PLy_procedure_name(PLyProce
*** 383,389 ****
static void
PLy_elog(int, const char *,...)
__attribute__((format(PG_PRINTF_ATTRIBUTE, 2, 3)));
! static void PLy_get_spi_error_data(PyObject *exc, char **detail, char **hint, char **query, int *position);
static void PLy_traceback(char **, char **, int *);
static void *PLy_malloc(size_t);
--- 383,389 ----
static void
PLy_elog(int, const char *,...)
__attribute__((format(PG_PRINTF_ATTRIBUTE, 2, 3)));
! static void PLy_get_spi_error_data(PyObject *exc, int *sqlerrcode, char **detail, char **hint, char **query, int *position);
static void PLy_traceback(char **, char **, int *);
static void *PLy_malloc(size_t);
*************** PLy_spi_exception_set(PyObject *excclass
*** 4441,4447 ****
if (!spierror)
goto failure;
! spidata = Py_BuildValue("(zzzi)", edata->detail, edata->hint,
edata->internalquery, edata->internalpos);
if (!spidata)
goto failure;
--- 4441,4447 ----
if (!spierror)
goto failure;
! spidata = Py_BuildValue("(izzzi)", edata->sqlerrcode, edata->detail, edata->hint,
edata->internalquery, edata->internalpos);
if (!spidata)
goto failure;
*************** PLy_elog(int elevel, const char *fmt,...
*** 4481,4486 ****
--- 4481,4487 ----
*val,
*tb;
const char *primary = NULL;
+ int sqlerrcode = 0;
char *detail = NULL;
char *hint = NULL;
char *query = NULL;
*************** PLy_elog(int elevel, const char *fmt,...
*** 4490,4496 ****
if (exc != NULL)
{
if (PyErr_GivenExceptionMatches(val, PLy_exc_spi_error))
! PLy_get_spi_error_data(val, &detail, &hint, &query, &position);
else if (PyErr_GivenExceptionMatches(val, PLy_exc_fatal))
elevel = FATAL;
}
--- 4491,4497 ----
if (exc != NULL)
{
if (PyErr_GivenExceptionMatches(val, PLy_exc_spi_error))
! PLy_get_spi_error_data(val, &sqlerrcode, &detail, &hint, &query, &position);
else if (PyErr_GivenExceptionMatches(val, PLy_exc_fatal))
elevel = FATAL;
}
*************** PLy_elog(int elevel, const char *fmt,...
*** 4531,4537 ****
PG_TRY();
{
ereport(elevel,
! (errmsg_internal("%s", primary ? primary : "no exception data"),
(detail) ? errdetail_internal("%s", detail) : 0,
(tb_depth > 0 && tbmsg) ? errcontext("%s", tbmsg) : 0,
(hint) ? errhint("%s", hint) : 0,
--- 4532,4539 ----
PG_TRY();
{
ereport(elevel,
! (errcode(sqlerrcode ? sqlerrcode : ERRCODE_INTERNAL_ERROR),
! errmsg_internal("%s", primary ? primary : "no exception data"),
(detail) ? errdetail_internal("%s", detail) : 0,
(tb_depth > 0 && tbmsg) ? errcontext("%s", tbmsg) : 0,
(hint) ? errhint("%s", hint) : 0,
*************** PLy_elog(int elevel, const char *fmt,...
*** 4562,4568 ****
* Extract the error data from a SPIError
*/
static void
! PLy_get_spi_error_data(PyObject *exc, char **detail, char **hint, char **query, int *position)
{
PyObject *spidata = NULL;
--- 4564,4570 ----
* Extract the error data from a SPIError
*/
static void
! PLy_get_spi_error_data(PyObject *exc, int* sqlerrcode, char **detail, char **hint, char **query, int *position)
{
PyObject *spidata = NULL;
*************** PLy_get_spi_error_data(PyObject *exc, ch
*** 4570,4576 ****
if (!spidata)
goto cleanup;
! if (!PyArg_ParseTuple(spidata, "zzzi", detail, hint, query, position))
goto cleanup;
cleanup:
--- 4572,4578 ----
if (!spidata)
goto cleanup;
! if (!PyArg_ParseTuple(spidata, "izzzi", sqlerrcode, detail, hint, query, position))
goto cleanup;
cleanup:
diff --git a/src/pl/plpython/sql/plpython_error.sql b/src/pl/plpython/sql/plpython_error.sql
index 4add6aa..502bbec 100644
*** a/src/pl/plpython/sql/plpython_error.sql
--- b/src/pl/plpython/sql/plpython_error.sql
*************** SELECT specific_exception(2);
*** 257,262 ****
--- 257,282 ----
SELECT specific_exception(NULL);
SELECT specific_exception(2);
+ /* SPI errors in PL/Python functions should preserve the SQLSTATE value
+ */
+ CREATE FUNCTION python_unique_violation() RETURNS void AS $$
+ plpy.execute("insert into specific values (1)")
+ plpy.execute("insert into specific values (1)")
+ $$ LANGUAGE plpythonu;
+
+ CREATE FUNCTION catch_python_unique_violation() RETURNS text AS $$
+ begin
+ begin
+ perform python_unique_violation();
+ exception when unique_violation then
+ return 'ok';
+ end;
+ return 'not reached';
+ end;
+ $$ language plpgsql;
+
+ SELECT catch_python_unique_violation();
+
/* manually starting subtransactions - a bad idea
*/
CREATE FUNCTION manual_subxact() RETURNS void AS $$
* Re: PL/Python SQL error code pass-through
@ 2011-11-24 15:15 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
parent: Jan Urbański <wulczer@wulczer.org>
0 siblings, 1 reply; 5+ messages in thread
From: Heikki Linnakangas @ 2011-11-24 15:15 UTC (permalink / raw)
To: Jan Urbański <wulczer@wulczer.org>; +Cc: Mika Eloranta <mel@ohmu.fi>; pgsql-hackers
On 24.11.2011 10:07, Jan Urbański wrote:
> On 23/11/11 17:24, Mika Eloranta wrote:
>> Hi all,
>>
>> [PL/Python in 9.1 does not preserve SQLSTATE of errors]
>
> Oops, you're right, it's a regression from 9.0 behaviour.
>
> The fix looks good to me, I changed one place to indent with tabs
> instead of spaces and added a regression test.
Thank you, both. Is there some other fields that we should propagate
from the original error message that we're missing? Like, context and
file/line information? Or are those left out on purpose? I wonder if we
should have a more wholesale approach, and store the whole ErrorData
struct somewhere, and only add some extra context information with
errcontext().
> I think this should be backpatched to 9.1, no?
Yeah, it should. Your patch probably makes most sense for backpatching,
even if we do something more radical on master.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: PL/Python SQL error code pass-through
@ 2011-11-24 21:56 Jan Urbański <wulczer@wulczer.org>
parent: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
0 siblings, 1 reply; 5+ messages in thread
From: Jan Urbański @ 2011-11-24 21:56 UTC (permalink / raw)
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>; +Cc: Mika Eloranta <mel@ohmu.fi>; pgsql-hackers
On 24/11/11 16:15, Heikki Linnakangas wrote:
> On 24.11.2011 10:07, Jan Urbański wrote:
>> On 23/11/11 17:24, Mika Eloranta wrote:
>>> Hi all,
>>>
>>> [PL/Python in 9.1 does not preserve SQLSTATE of errors]
>>
>> Oops, you're right, it's a regression from 9.0 behaviour.
>>
>> The fix looks good to me, I changed one place to indent with tabs
>> instead of spaces and added a regression test.
>
> Thank you, both. Is there some other fields that we should propagate
> from the original error message that we're missing? Like, context and
> file/line information? Or are those left out on purpose? I wonder if we
> should have a more wholesale approach, and store the whole ErrorData
> struct somewhere, and only add some extra context information with
> errcontext().
In case of SPI errors we're preserving the following from the original
ErrorData:
* sqlerrcode (as of Mika's patch)
* detail
* hint
* query
* internalpos
that leaves us with the following which are not preserved:
* message
* context
* detail_log
The message is being constructed from the Python exception name and I
think that's useful. The context is being taken by the traceback string.
I'm not sure if detail_log is ever set in these types of errors,
probably not? So I guess we're safe.
The problem with storing the entire ErrorData struct is that this
information has to be transformed to Python objects, because we attach
it to the Python exception that gets raised and in case it bubbles all
the way up to the topmost PL/Python function, we recover these Python
objects and use them to construct the ereport call. While the exception
is inside Python, user code can interact with it, so it'd be hard to
have C pointers to non-Python stuff there.
Cheers,
Jan
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: PL/Python SQL error code pass-through
@ 2011-12-02 21:22 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
parent: Jan Urbański <wulczer@wulczer.org>
0 siblings, 0 replies; 5+ messages in thread
From: Heikki Linnakangas @ 2011-12-02 21:22 UTC (permalink / raw)
To: Jan Urbański <wulczer@wulczer.org>; +Cc: Mika Eloranta <mel@ohmu.fi>; pgsql-hackers
On 24.11.2011 23:56, Jan Urbański wrote:
> On 24/11/11 16:15, Heikki Linnakangas wrote:
>> On 24.11.2011 10:07, Jan Urbański wrote:
>>> On 23/11/11 17:24, Mika Eloranta wrote:
>>>> [PL/Python in 9.1 does not preserve SQLSTATE of errors]
>>>
>>> Oops, you're right, it's a regression from 9.0 behaviour.
>>>
>>> The fix looks good to me, I changed one place to indent with tabs
>>> instead of spaces and added a regression test.
(Forgot to mention earlier: I committed the patch to master and
REL9_1_STABLE)
> In case of SPI errors we're preserving the following from the original
> ErrorData:
>
> * sqlerrcode (as of Mika's patch)
> * detail
> * hint
> * query
> * internalpos
>
> that leaves us with the following which are not preserved:
>
> * message
> * context
> * detail_log
>
> The message is being constructed from the Python exception name and I
> think that's useful. The context is being taken by the traceback string.
> I'm not sure if detail_log is ever set in these types of errors,
> probably not? So I guess we're safe.
Ok.
> The problem with storing the entire ErrorData struct is that this
> information has to be transformed to Python objects, because we attach
> it to the Python exception that gets raised and in case it bubbles all
> the way up to the topmost PL/Python function, we recover these Python
> objects and use them to construct the ereport call. While the exception
> is inside Python, user code can interact with it, so it'd be hard to
> have C pointers to non-Python stuff there.
Hmm, can the user also change the fields in the exception within python
code, or are they read-only? Is the spidata attribute in the exception
object visible to user code?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2011-12-02 21:22 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2011-11-23 16:24 PL/Python SQL error code pass-through Mika Eloranta <mel@ohmu.fi>
2011-11-24 08:07 ` Jan Urbański <wulczer@wulczer.org>
2011-11-24 15:15 ` Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
2011-11-24 21:56 ` Jan Urbański <wulczer@wulczer.org>
2011-12-02 21:22 ` Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox