Re: Revised patches to add table function support to PL/Tcl (TODO item)

On 02/08/2011 08:37 PM, Andrew Dunstan wrote:
>
>
> On 02/07/2011 11:30 PM, Robert Haas wrote:
>> On Tue, Dec 28, 2010 at 9:23 PM, Karl Lehenbauer
>> <karllehenbauer@gmail.com>  wrote:
>>> On Dec 28, 2010, at 7:29 PM, Tom Lane wrote:
>>>> This patch appears to be changing a whole lot of stuff that in fact
>>>> pg_indent has never changed, so there's something wrong with the 
>>>> way you
>>>> are doing it.  It looks like a bad typedef list from here.
>>> You were right, Tom.  The problem was that typedefs 
>>> "pltcl_interp_desc", "pltcl_proc_key", and "pltcl_proc_ptr" weren't 
>>> in src/tools/pgindent/typedefs.list.  After adding them (and 
>>> building and installing the netbsd-based, patched indent), pgindent 
>>> only changes a handful of lines.
>>>
>>> pltcl-karl-try3-1-of-3-pgindent.patch patches typedefs.list with the 
>>> three missing typedefs and pltcl.c with the small changes made by 
>>> pgindent (it shifted some embedded comments left within their lines, 
>>> mainly).
>>>
>>> As before, but "try3" now, pltcl-karl-try3-2-of-3-objects.patch 
>>> converts pltcl.c to use the "Tcl objects" C API.
>>>
>>> And as before, but "try3" now, pltcl-karl-try3-3-of-3-setof.patch 
>>> adds returning record and SETOF record.
>> This patch did not get reviewed, because the person who originally
>> planned to review it had a hardware failure that prevented him from
>> doing so.  Can anyone pick this up?
>
> I will have a look at it.
>
>


As promised I have had a look. The first point is that it doesn't have 
any documentation at all.

The second is that it doesn't appear from a my admittedly short look to 
support nested composites, or perhaps more importantly composites with 
array fields. I think if we're going to add support for composites to 
pltcl, we should make sure we support these from the start rather than 
store up for ourselves the sorts of trouble that we're now grappling 
with in plperl-land. We shouldn't start to make pltcl users pass back 
composed array or record literals, if possible.

As for the API changes, I'd like to have that piece reviewed by someone 
more familiar with the Tcl API than I am. I'm not sure who if anyone we 
have that has that familiarity, now Jan is no longer active.

I know this has been on the table for six weeks, and an earlier review 
might have given Karl more chance to remedy these matters in time. I'm 
sorry about that, it's a pity the original reviewer ran into issues.  
But for now I'm inclined to mark this as "Returned with Feedbnack".

cheers

andrew