Re: [HACKERS] More on GROUP BY

jwieck@debis.com (Jan Wieck) writes:
>     While  looking  at all these parsetrees I wonder why the hell
>     the GroupClause contains a complete copy of the TLE  at  all?
>     The  planner  depends on finding a corresponding entry in the
>     targetlist which should contain the same expression. At least
>     it needs an equal junk TLE. For the query
>         SELECT a, b FROM t1 GROUP BY b + 1;
>     the  parser  in  fact creates 3 TLE's where the last one is a
>     junk result named "resjunk" for the "b +  1"  expression  and
>     the GroupClause contains a totally equal TLE.

>     Could someone explain that please?

All true, but so what?  It wastes a few bytes of memory during
planning, I suppose...

>     Wouldn't it be better to have another field (resgroupno e.g.)
>     in the resdom  which  the  GroupClause  can  reference?  Then
>     changing  the resno's or even replacing the entire expression
>     wouldn't hurt because  make_subplanTargetList()  could  match
>     them  this  way  and  the expressions for the subplans can be
>     pulled out directly from the targetlist. And  it  would  save
>     processing  the  group  clauses in the rewriting because they
>     cannot contain Var nodes anymore and the entire list  can  be
>     ignored.

I think I like better the idea of leaving the representation alone,
but using equal() on the exprs to match groupclause items to targetlist
entries.  That way, manipulation of the targetlist can't accidentally
cause the grouplist to look like it contains something different than
what it should have.  It doesn't bother me that the planner can fail
if it is unable to match a group item to a targetlist item --- that's
a good crosscheck that nothing's gone wrong.  (But matching on just
the resno is unsafe, as you said before.)

I think it's true that using a TLE for each grouplist item is a waste of
space, and that representing the grouplist as simply a list of expr's
would be good enough.  But pulling out the TLE decoration seems like
it's not an appropriate use of time at this stage of the release cycle.
I'd say hold off till after 6.5, then fold it in with the parsetree
redesign that you keep muttering we need (I agree!).

BTW, you keep using the term "RTE" which I'm not familiar with ---
I assume it's just referring to the parse tree nodes?
        regards, tom lane