Re: [HACKERS] More on GROUP BY

jwieck(at)debis(dot)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