Hi.
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:
commit db7386187f78dfc45b86b6f4f382f6b12cdbc693
Author: Peter Eisentraut <pete...@gmx.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:
+ else
+ {
+ 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:
commit 09130e5867d49c72ef0f11bef30c5385d83bf194
Author: Tom Lane <t...@sss.pgh.pa.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
> excluded?
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
comments.)
I think the patch is ready for committer.
-- Abhijit
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
something.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
