Per recent discussion on -hackers, we should sometimes reorder the

columns of the grouping clause to avoid redundant sorts. The optimizer
is not currently capable of doing this, so this patch implements a
simple hack in the analysis phase (transformGroupClause): if any
subset of the GROUP BY clause matches a prefix of the ORDER BY list,
that prefix is moved to the front of the GROUP BY clause. This
shouldn't change the semantics of the query, and allows a redundant
sort to be avoided for queries like "GROUP BY a, b ORDER BY b".

diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 9fe07bb6fd5b8d97d70f8bf283c880a183640d7c..fdc2e41b3aaf03a5f0e604d83baf3e857be06392 100644 (file)
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/parser/parse_clause.c,v 1.146 2006/03/05 15:58:33 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/parser/parse_clause.c,v 1.147 2006/03/05 21:34:34 neilc Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1296,6 +1296,16 @@ findTargetlistEntry(ParseState *pstate, Node *node, List **tlist, int clause)
        return target_result;
 }
 
+static GroupClause *
+make_group_clause(TargetEntry *tle, List *targetlist, Oid sortop)
+{
+       GroupClause *result;
+
+       result = makeNode(GroupClause);
+       result->tleSortGroupRef = assignSortGroupRef(tle, targetlist);
+       result->sortop = sortop;
+       return result;
+}
 
 /*
  * transformGroupClause -
@@ -1303,71 +1313,105 @@ findTargetlistEntry(ParseState *pstate, Node *node, List **tlist, int clause)
  *
  * GROUP BY items will be added to the targetlist (as resjunk columns)
  * if not already present, so the targetlist must be passed by reference.
+ *
+ * The order of the elements of the grouping clause does not affect
+ * the semantics of the query. However, the optimizer is not currently
+ * smart enough to reorder the grouping clause, so we try to do some
+ * primitive reordering here.
  */
 List *
 transformGroupClause(ParseState *pstate, List *grouplist,
                                         List **targetlist, List *sortClause)
 {
-       List       *glist = NIL;
-       ListCell   *gl;
-       ListCell   *sortItem;
-
-       sortItem = list_head(sortClause);
+       List       *result = NIL;
+       List       *tle_list = NIL;
+       ListCell   *l;
 
-       foreach(gl, grouplist)
+       /* Preprocess the grouping clause, lookup TLEs */
+       foreach (l, grouplist)
        {
                TargetEntry *tle;
-               Oid                     restype;
-               Oid                     ordering_op;
-               GroupClause *grpcl;
+               Oid                      restype;
 
-               tle = findTargetlistEntry(pstate, lfirst(gl),
+               tle = findTargetlistEntry(pstate, lfirst(l),
                                                                  targetlist, GROUP_CLAUSE);
 
-               /* avoid making duplicate grouplist entries */
-               if (targetIsInSortList(tle, glist))
-                       continue;
-
                /* if tlist item is an UNKNOWN literal, change it to TEXT */
                restype = exprType((Node *) tle->expr);
 
                if (restype == UNKNOWNOID)
-               {
                        tle->expr = (Expr *) coerce_type(pstate, (Node *) tle->expr,
                                                                                         restype, TEXTOID, -1,
                                                                                         COERCION_IMPLICIT,
                                                                                         COERCE_IMPLICIT_CAST);
-                       restype = TEXTOID;
-               }
 
-               /*
-                * If the GROUP BY clause matches the ORDER BY clause, we want to
-                * adopt the ordering operators from the latter rather than using the
-                * default ops.  This allows "GROUP BY foo ORDER BY foo DESC" to be
-                * done with only one sort step.  Note we are assuming that any
-                * user-supplied ordering operator will bring equal values together,
-                * which is all that GROUP BY needs.
-                */
-               if (sortItem &&
-                       ((SortClause *) lfirst(sortItem))->tleSortGroupRef ==
-                       tle->ressortgroupref)
-               {
-                       ordering_op = ((SortClause *) lfirst(sortItem))->sortop;
-                       sortItem = lnext(sortItem);
-               }
-               else
+               tle_list = lappend(tle_list, tle);
+       }
+
+       /*
+        * Now iterate through the ORDER BY clause. If we find a grouping
+        * element that matches the ORDER BY element, append the grouping
+        * element to the result set immediately. Otherwise, stop
+        * iterating. The effect of this is to look for a prefix of the
+        * ORDER BY list in the grouping clauses, and to move that prefix
+        * to the front of the GROUP BY.
+        */
+       foreach (l, sortClause)
+       {
+               SortClause      *sc = (SortClause *) lfirst(l);
+               ListCell        *prev = NULL;
+               ListCell        *tl;
+               bool             found = false;
+
+               foreach (tl, tle_list)
                {
-                       ordering_op = ordering_oper_opid(restype);
-                       sortItem = NULL;        /* disregard ORDER BY once match fails */
+                       TargetEntry *tle = (TargetEntry *) lfirst(tl);
+
+                       if (sc->tleSortGroupRef == tle->ressortgroupref)
+                       {
+                               GroupClause *gc;
+
+                               tle_list = list_delete_cell(tle_list, tl, prev);
+
+                               /* Use the sort clause's sorting operator */
+                               gc = make_group_clause(tle, *targetlist, sc->sortop);
+                               result = lappend(result, gc);
+                               found = true;
+                               break;
+                       }
+
+                       prev = tl;
                }
 
-               grpcl = makeNode(GroupClause);
-               grpcl->tleSortGroupRef = assignSortGroupRef(tle, *targetlist);
-               grpcl->sortop = ordering_op;
-               glist = lappend(glist, grpcl);
+               /* As soon as we've failed to match an ORDER BY element, stop */
+               if (!found)
+                       break;
        }
 
-       return glist;
+       /*
+        * Now add any remaining elements of the GROUP BY list in the
+        * order we received them.
+        *
+        * XXX: are there any additional criteria to consider when
+        * ordering grouping clauses?
+        */
+       foreach(l, tle_list)
+       {
+               TargetEntry *tle = (TargetEntry *) lfirst(l);
+               GroupClause *gc;
+               Oid                      sort_op;
+
+               /* avoid making duplicate grouplist entries */
+               if (targetIsInSortList(tle, result))
+                       continue;
+
+               sort_op = ordering_oper_opid(exprType((Node *) tle->expr));
+               gc = make_group_clause(tle, *targetlist, sort_op);
+               result = lappend(result, gc);
+       }
+
+       list_free(tle_list);
+       return result;
 }
 
 /*