Re: [PATCH] GROUP BY ALL

David Christensen <david@pgguru.net> writes:
> Version 3 of this patch, incorporating some of the feedback thus far:

Some random comments:

I don't love where you put the parsing code.  Instead of
exposing addTargetToGroupList, I think you should have given
transformGroupClause the responsibility of expanding GROUP BY ALL.
One reason for that is that GROUP BY processing depends on the
results of transformSortClause.  This means that in v3, the
behavior of

     SELECT x FROM ... GROUP BY x ORDER BY x;

will be subtly different from

     SELECT x FROM ... GROUP BY ALL ORDER BY x;

which seems like a bug, or at least not desirable.  (You might need a
DESC or USING decoration in the ORDER BY to expose this clearly.)

The parsing code itself is not great:

+            TargetEntry *n = (TargetEntry*)lfirst(l1);
+            if (!contain_aggs_of_level((Node *)n->expr, 0))
+                qry->groupClause = addTargetToGroupList(pstate, n, qry->groupClause, qry->targetList, 0);

You should be skipping resjunk entries, and please use "tle" or some
such name less generic than "n", and "0" is not the correct location
to pass to addTargetToGroupList.  Probably the location of the ALL
keyword would be the ideal thing, but if we don't have that, the
notation for "no location known" is -1 not 0.  We could also
consider using exprLocation(tle->expr), despite the comment on
addTargetToGroupList that that's not the right thing.  This might
actually be better than pointing at the ALL anyway, since that would
give no hint which targetentry caused the error.

The test cases seem poorly designed, because it's very hard to be
sure whether the code expanded the ALL as-expected.  I think you
could improve them by not executing the queries but just EXPLAINing
them, so that the expanded group-by list is directly visible.

            regards, tom lane