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