Re: [HACKERS] PL/Python SQL error code pass-through
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 -- 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] PL/Python SQL error code pass-through
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
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;
+ intsqlerrcode = 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
Re: [HACKERS] PL/Python SQL error code pass-through
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 -- 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] PL/Python SQL error code pass-through
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PL/Python SQL error code pass-through
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 0001-PL-Python-SQL-error-code-pass-through.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
