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

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