Re: Revised patches to add table function support to PL/Tcl (TODO item) - Mailing list pgsql-hackers

From Andrew Dunstan
Subject Re: Revised patches to add table function support to PL/Tcl (TODO item)
Date
Msg-id 4D545745.9000304@dunslane.net
Whole thread Raw
In response to Re: Revised patches to add table function support to PL/Tcl (TODO item)  (Andrew Dunstan <andrew@dunslane.net>)
List pgsql-hackers

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


pgsql-hackers by date:

Previous
From: Robert Haas
Date:
Subject: Re: ALTER EXTENSION UPGRADE, v3
Next
From: "David E. Wheeler"
Date:
Subject: Re: ALTER EXTENSION UPGRADE, v3