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

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(at)poczta(dot)onet(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Hello

there is final Wojciech's patch

Regards

Pavel Stehule

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

Hello

2011/10/28 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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