Re: Review: [PL/pgSQL] %TYPE and array declaration - second patch - Mailing list pgsql-hackers
From | Pavel Stehule |
---|---|
Subject | Re: Review: [PL/pgSQL] %TYPE and array declaration - second patch |
Date | |
Msg-id | CAFj8pRDNQTLhxBvUi5hMn1zaW=Tp0Hz7MDhLcS2+txUbU9_Q9w@mail.gmail.com Whole thread Raw |
In response to | Re: Review: [PL/pgSQL] %TYPE and array declaration - second patch (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: Review: [PL/pgSQL] %TYPE and array declaration - second patch
|
List | pgsql-hackers |
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 >
pgsql-hackers by date: