Thread

1. Patch to add table function support to PL/Tcl (Todo item)

   Karl Lehenbauer <karllehenbauer@gmail.com> — 2010-12-28T15:33:42Z

   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.
   
   
   

2. Re: Patch to add table function support to PL/Tcl (Todo item)

   Alvaro Herrera <alvherre@commandprompt.com> — 2010-12-28T17:12:07Z

   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@commandprompt.com>
   The PostgreSQL Company - Command Prompt, Inc.
   PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   
   

3. Re: Patch to add table function support to PL/Tcl (Todo item)

   Karl Lehenbauer <karllehenbauer@gmail.com> — 2010-12-28T21:52:44Z

   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@commandprompt.com>
   > The PostgreSQL Company - Command Prompt, Inc.
   > PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   
   
   

4. Revised patches to add table function support to PL/Tcl (TODO item)

   Karl Lehenbauer <karllehenbauer@gmail.com> — 2010-12-28T22:56:23Z

   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.
   
   

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

   Tom Lane <tgl@sss.pgh.pa.us> — 2010-12-29T00:29:42Z

   Karl Lehenbauer <karllehenbauer@gmail.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
   
   

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

   Karl Lehenbauer <karllehenbauer@gmail.com> — 2010-12-29T02:23:58Z

   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.
   
   

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

   Robert Haas <robertmhaas@gmail.com> — 2011-02-08T04:30:49Z

   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?
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

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

   Andrew Dunstan <andrew@dunslane.net> — 2011-02-09T01:37:57Z

   
   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.
   
   cheers
   
   andrew
   
   

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

   Andrew Dunstan <andrew@dunslane.net> — 2011-02-10T21:23:17Z

   
   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