Thread

1. Review: [PL/pgSQL] %TYPE and array declaration - second patch

   Pavel Stehule <pavel.stehule@gmail.com> — 2011-10-23T17:33:56Z

   Hello
   
   I did a review this final patch
   
   1) we want this feature - it is long time in ToDo
   2) this feature support all types where it has sense
   3) patch respect PostgreSQL's coding standards
   4) patch was applied cleanly
   5) code was compiled without warnings
   6) regress tests was passed
   
   This patch is ready for commit
   
   I fixed a few  comments
   
   Regards
   
   Pavel Stehule
   
   
   2011/10/23 Wojciech Muła <wojciech_mula@poczta.onet.pl>:
   > Hi
   >
   > Pavel Stehule reviewed my first patch and pointed many
   > issues - thanks! This patch resolves three problems:
   >
   > 1. The main issue -  variables with copied types (%TYPE
   >   and %ROWTYPE attributes) can be declared as arrays.
   >   Grammar has been extended to match new syntax.
   >
   > 2. It's possible to copy type from function argument of
   >   composite type[1].
   >
   > 3. Enabling copying types from wider range of objects,
   >   data type resolution takes into account more cases.
   >   For example it wasn't possible to declare variable
   >   of row type, and then copy type from field of such
   >   variable (something like: declare x table%ROWTYPE;
   >   y x.field%TYPE).
   >
   > Patch includes new test cases and few words in documentation.
   >
   > best regards
   > Wojciech Muła
   >
   > [1] http://stackoverflow.com/questions/7634704/declare-variable-of-composite-type-in-postgresql-using-type
   >
   >
   > --
   > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
   > To make changes to your subscription:
   > http://www.postgresql.org/mailpref/pgsql-hackers
   >
   >
   

2. Re: Review: [PL/pgSQL] %TYPE and array declaration - second patch

   Pavel Stehule <pavel.stehule@gmail.com> — 2011-10-25T16:58:50Z

   Hello
   
   there is final Wojciech's patch
   
   Regards
   
   Pavel Stehule
   

3. Re: Review: [PL/pgSQL] %TYPE and array declaration - second patch

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-10-28T04:28:21Z

   Pavel Stehule <pavel.stehule@gmail.com> writes:
   > there is final Wojciech's patch
   
   I looked this over a little bit, but I don't see an answer to the
   question I put on the commitfest entry: why is this being done in
   plpgsql, and not somewhere in the core code?  The core has also got
   the concept of %TYPE, and could stand to have support for attaching []
   notation to that.  I'm not entirely sure if anything could be shared,
   but it would sure be worth looking at.  I've wished for some time that
   %TYPE not be handled directly in read_datatype() at all, but rather in
   the core parse_datatype() function.  This would require passing some
   sort of callback function to parse_datatype() to let it know what
   variables can be referenced in %TYPE, but we have parser callback
   functions just like that already.  One benefit we could get that way
   is that the core meaning of %TYPE (type of a table field name) would
   still be available in plpgsql, if the name didn't match any declared
   plpgsql variable.
   
   However, assuming that we're sticking to just changing plpgsql for the
   moment ... I cannot escape the feeling that this is a large patch with a
   small patch struggling to get out.  It should not require 500 net new
   lines of code to provide this functionality, when all we're doing is
   looking up the array type whose element type is the type the existing
   code can derive.  I would have expected to see the grammar passing one
   extra flag to plpgsql_parse_cwordtype and plpgsql_parse_cwordrowtype
   routines that were little changed except for the addition of a step to
   find the array type.
   
   Another complaint is that the grammar changes assume that the first
   token of decl_datatype must be T_WORD or T_CWORD, which means that
   it fails for cases such as unreserved plpgsql keywords.  This example
   should work, and does work in 9.1:
   
   create domain hint as int;
   
   create or replace function foo() returns void as $$
   declare
     x hint;
   begin
   end
   $$ language plpgsql;
   
   but fails with this patch because "hint" is returned by the lexer as
   K_HINT not T_WORD.  You might be able to get around that by adding a
   production with unreserved_keyword --- but I'm unsure that that would
   cover every case that worked before, and in any case I do not see the
   point of changing the grammar production at all.  It gains almost
   nothing to have Bison parse the [] rather than doing it with C code in
   read_datatype().
   
   			regards, tom lane
   
   

4. Re: Review: [PL/pgSQL] %TYPE and array declaration - second patch

   Pavel Stehule <pavel.stehule@gmail.com> — 2011-10-28T04:43:50Z

   Hello
   
   2011/10/28 Tom Lane <tgl@sss.pgh.pa.us>:
   > Pavel Stehule <pavel.stehule@gmail.com> writes:
   >> there is final Wojciech's patch
   >
   
   this is just small note about length of this patch. This patch was
   significantly smaller then he solved problem with derivate types for
   compound types - it should to solve problem described in this thread
   
   http://stackoverflow.com/questions/7634704/declare-variable-of-composite-type-in-postgresql-using-type
   
   Regards
   
   Pavel Stehule
   
   
   
   > I looked this over a little bit, but I don't see an answer to the
   > question I put on the commitfest entry: why is this being done in
   > plpgsql, and not somewhere in the core code?  The core has also got
   > the concept of %TYPE, and could stand to have support for attaching []
   > notation to that.  I'm not entirely sure if anything could be shared,
   > but it would sure be worth looking at.  I've wished for some time that
   > %TYPE not be handled directly in read_datatype() at all, but rather in
   > the core parse_datatype() function.  This would require passing some
   > sort of callback function to parse_datatype() to let it know what
   > variables can be referenced in %TYPE, but we have parser callback
   > functions just like that already.  One benefit we could get that way
   > is that the core meaning of %TYPE (type of a table field name) would
   > still be available in plpgsql, if the name didn't match any declared
   > plpgsql variable.
   >
   > However, assuming that we're sticking to just changing plpgsql for the
   > moment ... I cannot escape the feeling that this is a large patch with a
   > small patch struggling to get out.  It should not require 500 net new
   > lines of code to provide this functionality, when all we're doing is
   > looking up the array type whose element type is the type the existing
   > code can derive.  I would have expected to see the grammar passing one
   > extra flag to plpgsql_parse_cwordtype and plpgsql_parse_cwordrowtype
   > routines that were little changed except for the addition of a step to
   > find the array type.
   >
   > Another complaint is that the grammar changes assume that the first
   > token of decl_datatype must be T_WORD or T_CWORD, which means that
   > it fails for cases such as unreserved plpgsql keywords.  This example
   > should work, and does work in 9.1:
   >
   > create domain hint as int;
   >
   > create or replace function foo() returns void as $$
   > declare
   >  x hint;
   > begin
   > end
   > $$ language plpgsql;
   >
   > but fails with this patch because "hint" is returned by the lexer as
   > K_HINT not T_WORD.  You might be able to get around that by adding a
   > production with unreserved_keyword --- but I'm unsure that that would
   > cover every case that worked before, and in any case I do not see the
   > point of changing the grammar production at all.  It gains almost
   > nothing to have Bison parse the [] rather than doing it with C code in
   > read_datatype().
   >
   >                        regards, tom lane
   >
   
   

5. Re: Review: [PL/pgSQL] %TYPE and array declaration - second patch

   Tom Lane <tgl@sss.pgh.pa.us> — 2011-10-28T17:06:15Z

   Pavel Stehule <pavel.stehule@gmail.com> writes:
   > this is just small note about length of this patch. This patch was
   > significantly smaller then he solved problem with derivate types for
   > compound types - it should to solve problem described in this thread
   
   > http://stackoverflow.com/questions/7634704/declare-variable-of-composite-type-in-postgresql-using-type
   
   Well, I think what that example shows is that there's a good reason for
   plpgsql_parse_wordtype and plpgsql_parse_cwordtype to handle the
   PLPGSQL_NSTYPE_ROW case, which they could do based on the tdtypeid from
   the row's tupdesc.  Still isn't going to run to anything like 500 lines
   of new code, nor justify a grammar rewrite that risks introducing new
   bugs.  The existing code doesn't need to special-case type names that
   are also plpgsql keywords, and I'd just as soon not introduce an
   assumption that there's no overlap there.
   
   			regards, tom lane