|From:||Abhijit Menon-Sen <ams(at)2ndQuadrant(dot)com>|
|To:||Sim Zacks <sim(at)compulab(dot)co(dot)il>|
|Cc:||ebehn(at)arinc(dot)com, pgsql-hackers(at)postgresql(dot)org, Peter Eisentraut <peter_e(at)gmx(dot)net>|
|Subject:||Re: Array of composite types returned from python|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
When this patch was first added to a CF, I had a quick look at it, but
left it for a proper review by someone more familiar with PL/Python
internals for precisely this reason:
> 2) You removed the comment:
> - /*
> - * We don't support arrays of row types yet, so the first argument
> - * can be NULL.
> - */
> But didn't change the code there.
> I haven't delved deep enough into the code yet to understand the full
> meaning, but the comment would indicate that if arrays of row types
> are supported, the first argument cannot be null.
I had another look now, and I think removing the comment is fine. It
actually made no sense to me in context, so I went digging a little.
After following a plpython.c → plpy_*.c refactoring (#147c2482) and a
pgindent run (#65e806cb), I found that the comment was added along with
the code by this commit:
Author: Peter Eisentraut <peter_e(at)gmx(dot)net>
Date: Thu Dec 10 20:43:40 2009 +0000
PL/Python array support
Support arrays as parameters and return values of PL/Python functions.
At the time, the code looked like this:
+ nulls[i] = false;
+ /* We don't support arrays of row types yet, so the first
+ * argument can be NULL. */
+ elems[i] = arg->elm->func(NULL, arg->elm, obj);
Note that the first argument was actually NULL, so the comment made
sense when it was written. But the code was subsequently changed to
pass in arg->elm by the following commit:
Author: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Date: Mon Oct 11 22:16:40 2010 -0400
Fix plpython so that it again honors typmod while assigning to tuple fields.
This was broken in 9.0 while improving plpython's conversion behavior for
bytea and boolean. Per bug report from maizi.
The comment should have been removed at the same time. So I don't think
there's a problem here.
> 3) This is such a simple change with no new infrastructure code
> (PLyObject_ToComposite already exists). Can you think of a reason
> why this wasn't done until now? Was it a simple miss or purposefully
This is not an authoritative answer: I think the infrastructure was
originally missing, but was later added in #bc411f25 for OUT parameters.
Perhaps it was overlooked at the time that the same code would suffice
for this earlier-missing case. (I've Cc:ed Peter E. in case he has any
I think the patch is ready for committer.
P.S. I'm a wee bit confused by this mail I'm replying to, because it's
signed "Ed" and looks like a response, but it's "From: Sim Zacks". I've
added the original author's address to the Cc: in case I misunderstood
|Next Message||Abhijit Menon-Sen||2014-06-29 12:43:14||Re: Array of composite types returned from python|
|Previous Message||Andres Freund||2014-06-29 12:25:44||Re: Cluster name in ps output|