Thread

1. plpython triggers are broken for composite-type columns

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-04-10T02:20:06Z

   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
   
   

2. Re: plpython triggers are broken for composite-type columns

   Jan Urbański <wulczer@wulczer.org> — 2012-04-10T05:35:45Z

   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
   
   

3. Re: plpython triggers are broken for composite-type columns

   Jan Urbański <wulczer@wulczer.org> — 2012-04-10T17:07:41Z

   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
   
   

4. Re: plpython triggers are broken for composite-type columns

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-04-10T18:35:44Z

   =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.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
   
   

5. Re: plpython triggers are broken for composite-type columns

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-04-10T18:47:27Z

   I wrote:
   > =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.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
   
   

6. Re: plpython triggers are broken for composite-type columns

   Jan Urbański <wulczer@wulczer.org> — 2012-04-10T19:21:49Z

   On 10/04/12 20:47, Tom Lane wrote:
   > I wrote:
   >> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?=<wulczer@wulczer.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?
   
   

7. Re: plpython triggers are broken for composite-type columns

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-04-10T19:27:37Z

   =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.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
   
   

8. Re: plpython triggers are broken for composite-type columns

   Jan Urbański <wulczer@wulczer.org> — 2012-04-10T19:47:32Z

   On 10/04/12 21:27, Tom Lane wrote:
   > =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?=<wulczer@wulczer.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
   
   

9. Re: plpython triggers are broken for composite-type columns

   Jan Urbański <wulczer@wulczer.org> — 2012-04-23T00:25:41Z

   On 10/04/12 21:47, Jan Urbański wrote:
   > On 10/04/12 21:27, Tom Lane wrote:
   >> =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?=<wulczer@wulczer.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
   

10. Re: plpython triggers are broken for composite-type columns

    Peter Eisentraut <peter_e@gmx.net> — 2012-04-26T18:13:41Z

    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.