Re: plpython triggers are broken for composite-type columns

Don't know if anybody noticed bug #6559
http://archives.postgresql.org/pgsql-bugs/2012-03/msg00180.php

I've confirmed that the given test case works in 9.0 but fails in
9.1 and HEAD. It's not terribly sensitive to the details of the
SQL: any non-null value for the composite column fails, for instance
you can try
INSERT INTO tbl VALUES (row(3), 4);
and it spits up just the same. The long and the short of it is that
PLy_modify_tuple fails to make sense of what PLyDict_FromTuple
produced for the table row.

I tried to trace through things to see exactly where it was going wrong,
and noted that

(1) When converting the table row to a Python dict, the composite
column value is fed through the generic PLyString_FromDatum() function,
which seems likely to be the wrong choice.

(2) When converting back, the composite column value is routed to
PLyObject_ToTuple, which decides it is a Python sequence, which seems
a bit improbable considering it was merely a string a moment ago.

I find this code pretty unreadable, though, and know nothing to
speak of about the Python side of things anyhow. So somebody else
had better pick this up.

regards, tom lane

On 10/04/12 04:20, Tom Lane wrote:
> Don't know if anybody noticed bug #6559
> http://archives.postgresql.org/pgsql-bugs/2012-03/msg00180.php
>
> I've confirmed that the given test case works in 9.0 but fails in
> 9.1 and HEAD.
>
> I find this code pretty unreadable, though, and know nothing to
> speak of about the Python side of things anyhow. So somebody else
> had better pick this up.

I'll look into that.

Cheers,
Jan

On 10/04/12 07:35, Jan Urbański wrote:
> On 10/04/12 04:20, Tom Lane wrote:
>> Don't know if anybody noticed bug #6559
>> http://archives.postgresql.org/pgsql-bugs/2012-03/msg00180.php
>>
>> I've confirmed that the given test case works in 9.0 but fails in
>> 9.1 and HEAD.

So, I know what's going on, I still don't know what's the best way to
handle it.

The function that converts Python objects to PG data checks what type
it's supposed to produce and acts accordingly. In 9.0 it checked for
bool, bytea and arrays, in 9.1 it also takes composite types into account.

This has been done to support functions returning composite types - to
do that they need to return a dictionary or a list, for instance
{'col1': 1, 'col2': 2}.

The problem is that the routine that converts PG data into Python
objects does not handle composite type inputs all that well - it just
bails and returns the string representation, hence '(3)' appearing in
Python land.

Now previously, the Python->PG function did not see that the given
conversion is supposed to return a composite so it also bailed and used
a default text->composite conversion, so '(3)' was converted to ROW(3)
and all went well. The new code tries to treat what it gets as a
dictionary/list/tuple and fails in a more or less random way.

Now that I understand what's been going on, I'll try to think of a
non-invasive way of fixing that...

Cheers,
Jan

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer(at)wulczer(dot)org> writes:
>> On 10/04/12 04:20, Tom Lane wrote:
>>> Don't know if anybody noticed bug #6559
>>> http://archives.postgresql.org/pgsql-bugs/2012-03/msg00180.php

> So, I know what's going on, I still don't know what's the best way to
> handle it.

> The function that converts Python objects to PG data checks what type
> it's supposed to produce and acts accordingly. In 9.0 it checked for
> bool, bytea and arrays, in 9.1 it also takes composite types into account.

> This has been done to support functions returning composite types - to
> do that they need to return a dictionary or a list, for instance
> {'col1': 1, 'col2': 2}.

> The problem is that the routine that converts PG data into Python
> objects does not handle composite type inputs all that well - it just
> bails and returns the string representation, hence '(3)' appearing in
> Python land.

> Now previously, the Python->PG function did not see that the given
> conversion is supposed to return a composite so it also bailed and used
> a default text->composite conversion, so '(3)' was converted to ROW(3)
> and all went well. The new code tries to treat what it gets as a
> dictionary/list/tuple and fails in a more or less random way.

> Now that I understand what's been going on, I'll try to think of a
> non-invasive way of fixing that...

ISTM that conversion of a composite value to Python ought to produce a
dict, now that the other direction expects a dict. I can see that this
is probably infeasible for compatibility reasons in 9.1, but it's not
too late to fix it for 9.2. We might have to leave the bug unfixed in
9.1, since anything we do about it will represent a compatibility break.

regards, tom lane

I wrote:
> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer(at)wulczer(dot)org> writes:
>> Now that I understand what's been going on, I'll try to think of a
>> non-invasive way of fixing that...

> ISTM that conversion of a composite value to Python ought to produce a
> dict, now that the other direction expects a dict. I can see that this
> is probably infeasible for compatibility reasons in 9.1, but it's not
> too late to fix it for 9.2. We might have to leave the bug unfixed in
> 9.1, since anything we do about it will represent a compatibility break.

On reflection, can't we fix this as follows: if the value coming in from
Python is a string, just feed it to record_in, the same as we used to.
When I traced through the logic before, it seemed like it was failing
to distinguish strings from sequences, but I would hope that Python
is more strongly typed than that.

I still think the conversion in the other direction ought to yield a
dict, but that's clearly not back-patch material.

regards, tom lane

On 10/04/12 20:47, Tom Lane wrote:
> I wrote:
>> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?=<wulczer(at)wulczer(dot)org> writes:
>>> Now that I understand what's been going on, I'll try to think of a
>>> non-invasive way of fixing that...
>
>> ISTM that conversion of a composite value to Python ought to produce a
>> dict, now that the other direction expects a dict. I can see that this
>> is probably infeasible for compatibility reasons in 9.1, but it's not
>> too late to fix it for 9.2. We might have to leave the bug unfixed in
>> 9.1, since anything we do about it will represent a compatibility break.
>
> On reflection, can't we fix this as follows: if the value coming in from
> Python is a string, just feed it to record_in, the same as we used to.
> When I traced through the logic before, it seemed like it was failing
> to distinguish strings from sequences, but I would hope that Python
> is more strongly typed than that.

Yeah, we can fix PLyObject_ToTuple to check for strings too and use the
default PG input function. The reason it was complaining about length is
that we're checking if the object passed implements the sequence
protocol, which Python strings do (length, iteration, etc). Sticking a
if branch that will catch the string case above that should be sufficient.

>
> I still think the conversion in the other direction ought to yield a
> dict, but that's clearly not back-patch material.

Yes, that would be ideal, even though not backwards-compatible.
Back-patching is out of the question, but do we want to change trigger
functions to receive dictionaries in NEW? If so, should this be 9.2
material, or just a TODO?

=?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer(at)wulczer(dot)org> writes:
> On 10/04/12 20:47, Tom Lane wrote:
>> On reflection, can't we fix this as follows: if the value coming in from
>> Python is a string, just feed it to record_in, the same as we used to.
>> When I traced through the logic before, it seemed like it was failing
>> to distinguish strings from sequences, but I would hope that Python
>> is more strongly typed than that.

> Yeah, we can fix PLyObject_ToTuple to check for strings too and use the
> default PG input function. The reason it was complaining about length is
> that we're checking if the object passed implements the sequence
> protocol, which Python strings do (length, iteration, etc). Sticking a
> if branch that will catch the string case above that should be sufficient.

Ah, makes sense then. (Perhaps the dict case ought to be tested before
the sequence case, too, just to be safe?)

>> I still think the conversion in the other direction ought to yield a
>> dict, but that's clearly not back-patch material.

> Yes, that would be ideal, even though not backwards-compatible.
> Back-patching is out of the question, but do we want to change trigger
> functions to receive dictionaries in NEW?

Hm, I was not thinking of this as being trigger-specific, but more a
general principle that composite columns of tuples ought to be handled
in a recursive fashion.

> If so, should this be 9.2 material, or just a TODO?

If it can be done quickly and with not much risk, I'd vote for
squeezing it into 9.2, because it seems to me to be a clear bug that the
two directions are not handled consistently. If you don't have time for
it now or you don't think it would be a small/safe patch, we'd better
just put it on TODO.

regards, tom lane

On 10/04/12 21:27, Tom Lane wrote:
> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?=<wulczer(at)wulczer(dot)org> writes:
>> Yes, that would be ideal, even though not backwards-compatible.
>> Back-patching is out of the question, but do we want to change trigger
>> functions to receive dictionaries in NEW?
>
> Hm, I was not thinking of this as being trigger-specific, but more a
> general principle that composite columns of tuples ought to be handled
> in a recursive fashion.

Sure, that would be the way.

>> If so, should this be 9.2 material, or just a TODO?
>
> If it can be done quickly and with not much risk, I'd vote for
> squeezing it into 9.2, because it seems to me to be a clear bug that the
> two directions are not handled consistently. If you don't have time for
> it now or you don't think it would be a small/safe patch, we'd better
> just put it on TODO.

I'll see if making the conversion function recursive is easy and
independently whip up a patch to check for strings and routes them
through InputFunctionCall, for back-patching purposes.

Cheers,
Jan

On 10/04/12 21:47, Jan Urbański wrote:
> On 10/04/12 21:27, Tom Lane wrote:
>> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?=<wulczer(at)wulczer(dot)org> writes:
>>> Yes, that would be ideal, even though not backwards-compatible.
>>> Back-patching is out of the question, but do we want to change trigger
>>> functions to receive dictionaries in NEW?
>>
>> Hm, I was not thinking of this as being trigger-specific, but more a
>> general principle that composite columns of tuples ought to be handled
>> in a recursive fashion.
>
> Sure, that would be the way.
>
>>> If so, should this be 9.2 material, or just a TODO?
>>
>> If it can be done quickly and with not much risk, I'd vote for
>> squeezing it into 9.2, because it seems to me to be a clear bug that the
>> two directions are not handled consistently. If you don't have time for
>> it now or you don't think it would be a small/safe patch, we'd better
>> just put it on TODO.

It turned out not to be as straightforward as I though :(

The I/O code in PL/Python is a bit of a mess and that's something that
I'd like to address somewhere in the 9.3 development cycle. Right now
making the conversion function recursive is not possible without some
deep surgery (or kludgery...) so I limited myself to producing
regression-fixing patches for 9.2 and 9.1 (attached).

Cheers,
Jan

On mån, 2012-04-23 at 02:25 +0200, Jan Urbański wrote:
> It turned out not to be as straightforward as I though :(

Yeah, been there ...
>
> The I/O code in PL/Python is a bit of a mess and that's something that
> I'd like to address somewhere in the 9.3 development cycle. Right now
> making the conversion function recursive is not possible without some
> deep surgery (or kludgery...) so I limited myself to producing
> regression-fixing patches for 9.2 and 9.1 (attached).

Committed.