Re: [HACKERS] Review: [PL/pgSQL] %TYPE and array declaration - second patch
Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: [PL/pgSQL] %TYPE and array declaration - second patch
Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: [PL/pgSQL] %TYPE and array declaration - second patch
Hello 2011/10/28 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
