Re: %TYPE and array declaration patch review

Hello

2011/11/28 Greg Smith <greg(at)2ndquadrant(dot)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

>

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 US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

2011/12/10 Greg Smith <greg(at)2ndquadrant(dot)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    greg(at)2ndQuadrant(dot)com   Baltimore, MD
> PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us
>
>
> --
> 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

On Sat, 10 Dec 2011 10:22:54 -0500 Greg Smith <greg(at)2ndQuadrant(dot)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