Re: 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

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

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

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

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