Re: [HACKERS] %TYPE and array declaration patch review
On 11/30/2011 10:42 AM, Pavel Stehule wrote: Regress tests are really large - it is question if about 900 lines is necessary - should be more compact Can't recall the last time I heard a complaint about having too many regression tests for new code. We've got some bit rot, code convention suggestions, test scope concerns, and some fundamental design questions hanging over this one. I don't think anyone expected this TODO item was going to turn into a 2626 line patch, but discovering a bunch of unexpected complexity underneath one of those is a regular event. Stuff that's straighforward to implement tends to just get done instead of added there. Tom's concerns about the grammar rewrite and way parsing is handled here seem the worst blockers for committing this, and I can't imagine how those could be resolved before this CommitFest is over. I'm going to mark this one as returned with that and Pavel's feedback. Perhaps Wojciech or someone else will come up with a clever way to address this, one that has less impact on existing code. Adding more parser complexity than absolutely necessary is a much bigger problem than the regression tests being too long. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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] %TYPE and array declaration patch review
2011/12/10 Greg Smith g...@2ndquadrant.com: On 11/30/2011 10:42 AM, Pavel Stehule wrote: Regress tests are really large - it is question if about 900 lines is necessary - should be more compact Can't recall the last time I heard a complaint about having too many regression tests for new code. We've got some bit rot, code convention suggestions, test scope concerns, and some fundamental design questions hanging over this one. I don't think anyone expected this TODO item was going to turn into a 2626 line patch, but discovering a bunch of unexpected complexity underneath one of those is a regular event. Stuff that's straighforward to implement tends to just get done instead of added there. this is just my opinion, it's not blocker (about regress tests length) but maybe this patch should be divided to some smaller parts Regards Pavel Tom's concerns about the grammar rewrite and way parsing is handled here seem the worst blockers for committing this, and I can't imagine how those could be resolved before this CommitFest is over. I'm going to mark this one as returned with that and Pavel's feedback. Perhaps Wojciech or someone else will come up with a clever way to address this, one that has less impact on existing code. Adding more parser complexity than absolutely necessary is a much bigger problem than the regression tests being too long. -- Greg Smith 2ndQuadrant US g...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- 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] %TYPE and array declaration patch review
On Sat, 10 Dec 2011 10:22:54 -0500 Greg Smith g...@2ndquadrant.com wrote: Tom's concerns about the grammar rewrite and way parsing is handled here seem the worst blockers for committing this, and I can't imagine how those could be resolved before this CommitFest is over. I'm going to mark this one as returned with that and Pavel's feedback. Perhaps Wojciech or someone else will come up with a clever way to address this, one that has less impact on existing code. Adding more parser complexity than absolutely necessary is a much bigger problem than the regression tests being too long. My first attempt to fix the issue was very short simple, I slightly modified existing C code to handle additional suffixes, see http://archives.postgresql.org/pgsql-hackers/2011-08/msg00998.php Pavel reviewed this and pointed that would be great if we were fix related issues. Thus, the second patch try to resolve: 1. handling array indices; 2. copying types; 3. handling %TYPE/%ROWTYPE in procedure definition. Ad 1 - IMHO first attempt is good enough :) (except coding style) Ad 2 - this is broken now, just few cases are handled correctly. My code handled more cases manually, but now I see this have to be solved with kind of generic pattern-matching procedure (maybe with backtracking) - I hope shorter, cleaner and easier to understand and maintain. Tom Lane wrote it would be good if resolving types were handled in core, but IMO only PL/pgsql code will use this procedure. Ad 3 - minor issue, could be fixed anytime So, my proposition is to drop current approach and back to the first proposition. Copying types have to be considered as separate issue. regards Wojciech Muła -- 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] %TYPE and array declaration patch review
Hello 2011/11/28 Greg Smith g...@2ndquadrant.com: I'm trying to find someone for the [PL/pgSQL] %TYPE and array declaration - second patch patch submitted recently: https://commitfest.postgresql.org/action/patch_view?id=666 Not too many people work on the PL/pgSQL code, and I see you reviewed an earlier version of this patch. Do you think you could find time to review the update to it as well? This patch is not applyed cleanly now bash-4.2$ patch -p1 type_array.patch patching file doc/src/sgml/plpgsql.sgml patching file src/pl/plpgsql/src/gram.y Hunk #5 succeeded at 2540 (offset -12 lines). Hunk #6 succeeded at 2554 (offset -12 lines). Hunk #7 succeeded at 2595 (offset -12 lines). patching file src/pl/plpgsql/src/pl_comp.c Hunk #1 succeeded at 1586 (offset 2 lines). Hunk #2 succeeded at 1883 (offset 2 lines). Hunk #3 FAILED at 1901. Hunk #4 succeeded at 2034 (offset 3 lines). 1 out of 4 hunks FAILED -- saving rejects to file src/pl/plpgsql/src/pl_comp.c.rej patching file src/pl/plpgsql/src/plpgsql.h Hunk #2 succeeded at 895 (offset 8 lines). patching file src/test/regress/expected/plpgsql.out patching file src/test/regress/sql/plpgsql.sql I dislike using macros without parameters +#define word1 strVal(linitial(idents)) +#define word2 strVal(lsecond(idents)) +#define word3 strVal(lthird(idents)) and - nse = plpgsql_ns_lookup(plpgsql_ns_top(), false, - strVal(linitial(idents)), - strVal(lsecond(idents)), - NULL, + var = (PLpgSQL_var *) plpgsql_get_variable2( + word1, + word2, + PLPGSQL_NSTYPE_VAR, + NULL); This change is useless, and smudges a code - a list operations are well known and is not neccessary hide it. macros #define linitial_str(lc) strVal(linitial(lc)) #define lsecond_str(lc) strVal(lsecond(lc)) #define lthird_str(lc) strVal(lthird(lc)) these macros should be defined only once per module - #undef is not used usually in pg source code, don't use it in this case Regress tests are really large - it is question if about 900 lines is necessary - should be more compact Regards Pavel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
