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

Project name: Add table function support to PL/Tcl (Todo item)

What the patch does:

This patch adds table function support (returning record and SETOF record)
to PL/Tcl. This patch also updates PL/Tcl to use the Tcl object-style
interface instead of the older string-style one, increasing performance.

Status of the patch:

The code seems to work well, but this is its first submission.

Branch the patch is against: HEAD

Compiles and tests successfully on FreeBSD and Mac OS X. Have not tested
it with other systems but there is nothing platform specific about it.

Regression tests: Passes all existing tests but there aren't many for PL/Tcl.

This change removes PL/Tcl backward compatibility to Tcl version 7.
Since Tcl 8 has been in production release since 1997, I felt
that 13 years was long enough and PL/Tcl users linking with Tcl 7 should
go ahead and upgrade. This also allowed removal of the Tcl 7 compatibility
shims.

More importantly, this patch extends PL/Tcl to support returning rows and
sets of rows. While I studied all of the other PL languages (PL/PgSql,
PL/Perl, PL/Python and PL/C) while developing this patch, it hews most
closely to to approach taken by PL/PgSQL.

All existing semantics for functions and triggers have been retained, requiring
no changes to existing PL/Tcl code.

PL/Tcl coders who want to create functions returning a record will use "return"
to return results, the same as for a scalar, except that the value returned
should be a list of key-value pairs ("array get" format) where the keys are
`the field names and the values are the corresponding values.

To return sets of rows, one needs to use the new PL/Tcl function "return_next".
Return_next also accepts a list of key-value pairs, as "return" does.

Typically this will be invoked as something like

return_next [array get row]

To return multiple rows, the function should invoke return_next
multiple times (once for each row returned). As mentioned, the C
implementation works like PL/PgSQL, so PL/Tcl saves up the tuples in a
tuple store and then uses the SFRM_Materialize return mode to send the
results back. Fields are converted to Datum during the call to return_next,
so if any field names are in the list that aren't in the row or there are
data conversion errors, they will be returned as a Tcl error to the caller of
return_next and can be caught using Tcl's "catch", etc.

Excerpts from Karl Lehenbauer's message of mar dic 28 12:33:42 -0300 2010:
> Project name: Add table function support to PL/Tcl (Todo item)
>
> What the patch does:
>
> This patch adds table function support (returning record and SETOF record)
> to PL/Tcl. This patch also updates PL/Tcl to use the Tcl object-style
> interface instead of the older string-style one, increasing performance.

While I don't use PL/Tcl myself, this seems a reasonable idea. However,
I think this patch does too many things in one step. It also contains
numerous superfluous whitespace changes that make it hard to assess its
real size.

I'd recommend splitting it up and dropping the whitespace changes (which
would be reverted by pgindent anyway).

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hmm, I ran the code through pgindent so I don't understand why there are whitespace changes.

OK I'll see what the problem is with the whitespace and instead produce two patches, one that converts to using Tcl objects and one on top of that that adds returning records and setof records.

On Dec 28, 2010, at 12:12 PM, Alvaro Herrera wrote:

> Excerpts from Karl Lehenbauer's message of mar dic 28 12:33:42 -0300 2010:
>> Project name: Add table function support to PL/Tcl (Todo item)
>>
>> What the patch does:
>>
>> This patch adds table function support (returning record and SETOF record)
>> to PL/Tcl. This patch also updates PL/Tcl to use the Tcl object-style
>> interface instead of the older string-style one, increasing performance.
>
> While I don't use PL/Tcl myself, this seems a reasonable idea. However,
> I think this patch does too many things in one step. It also contains
> numerous superfluous whitespace changes that make it hard to assess its
> real size.
>
> I'd recommend splitting it up and dropping the whitespace changes (which
> would be reverted by pgindent anyway).
>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support

In response to Alvaro Herrera's message from today I've split the PL/Tcl table function patch into three separate, easier-to-digest patches. (Thanks for the quick response, Alvaro.)

The first patch, pltcl-karl-try2-1-of-3-pgindent.patch, does nothing but conform HEAD's pltcl.c with pgindent. Applying this patch should have exactly the same effect as running
src/tools/pgindent/pgindent src/tools/pgindent/typedefs.list src/pl/tcl/pltcl.c

The second patch, pltcl-karl-try2-2-of-3-objects.patch, should be applied after the first, and updates PL/Tcl to use the Tcl "Tcl object" C API, the preferred way of interacting with Tcl from C since Tcl 8.0 was released in 1997.

The third patch, pltcl-karl-try2-3-of-3-setof.patch, builds on the above to add both the "return_next" command for returning multiple rows in a SETOF-returning function and to add using "return" with a list of key-value pairs for functions returning a non-SETOF record.

Karl Lehenbauer <karllehenbauer(at)gmail(dot)com> writes:
> The first patch, pltcl-karl-try2-1-of-3-pgindent.patch, does nothing but conform HEAD's pltcl.c with pgindent. Applying this patch should have exactly the same effect as running
> src/tools/pgindent/pgindent src/tools/pgindent/typedefs.list src/pl/tcl/pltcl.c

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.

regards, tom lane

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.

On Tue, Dec 28, 2010 at 9:23 PM, Karl Lehenbauer
<karllehenbauer(at)gmail(dot)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?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 02/07/2011 11:30 PM, Robert Haas wrote:
> On Tue, Dec 28, 2010 at 9:23 PM, Karl Lehenbauer
> <karllehenbauer(at)gmail(dot)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.

cheers

andrew

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(at)gmail(dot)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