]> granicus.if.org Git - postgresql/commitdiff
Replace generic 'Illegal use of aggregates' error message with one that
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 9 Dec 1999 05:58:56 +0000 (05:58 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 9 Dec 1999 05:58:56 +0000 (05:58 +0000)
shows the specific ungrouped variable being complained of.  Perhaps this
will reduce user confusion...

src/backend/optimizer/plan/planmain.c
src/backend/optimizer/plan/planner.c
src/backend/optimizer/util/clauses.c
src/backend/parser/parse_agg.c
src/include/optimizer/clauses.h
src/test/regress/expected/select_implicit.out

index 7da2ee4c211002630176ed1bba462198e572e065..7dcafe35dceaf340c346b4aab3b2d0ac19c253c3 100644 (file)
@@ -7,7 +7,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planmain.c,v 1.47 1999/11/15 02:00:07 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planmain.c,v 1.48 1999/12/09 05:58:52 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -100,10 +100,7 @@ query_planner(Query *root,
                         * Note we do NOT do this for subplans in WHERE; it's legal
                         * there because WHERE is evaluated pre-GROUP.
                         */
-                       if (check_subplans_for_ungrouped_vars((Node *) tlist,
-                                                                                                 root->groupClause,
-                                                                                                 tlist))
-                               elog(ERROR, "Sub-SELECT must use only GROUPed attributes from outer SELECT");
+                       check_subplans_for_ungrouped_vars((Node *) tlist, root, tlist);
                }
        }
 
index 295d722b6a55f0abe74b1840194a6bccd4c21057..278ef356f127a2a16fdd926684dadb4942dd939c 100644 (file)
@@ -7,7 +7,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v 1.71 1999/11/15 02:00:08 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v 1.72 1999/12/09 05:58:52 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -344,10 +344,9 @@ union_planner(Query *parse)
                        /* Expand SubLinks to SubPlans */
                        parse->havingQual = SS_process_sublinks(parse->havingQual);
                        /* Check for ungrouped variables passed to subplans */
-                       if (check_subplans_for_ungrouped_vars(parse->havingQual,
-                                                                                                 parse->groupClause,
-                                                                                                 parse->targetList))
-                               elog(ERROR, "Sub-SELECT must use only GROUPed attributes from outer SELECT");
+                       check_subplans_for_ungrouped_vars(parse->havingQual,
+                                                                                         parse,
+                                                                                         parse->targetList);
                }
        }
 
index 07ccca7bf020325ebad7d3a528fa2135189aa755..63b3ff87d9e298ffde33d9821eea02a33dbcd033 100644 (file)
@@ -7,7 +7,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.55 1999/11/22 17:56:17 momjian Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.56 1999/12/09 05:58:53 tgl Exp $
  *
  * HISTORY
  *       AUTHOR                        DATE                    MAJOR EVENT
@@ -30,6 +30,7 @@
 #include "optimizer/tlist.h"
 #include "optimizer/var.h"
 #include "parser/parse_type.h"
+#include "parser/parsetree.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
 
@@ -40,7 +41,7 @@
                                                (isnull), true, false, false))
 
 typedef struct {
-       List       *groupClause;
+       Query      *query;
        List       *targetList;
 } check_subplans_for_ungrouped_vars_context;
 
@@ -427,28 +428,30 @@ pull_agg_clause_walker(Node *node, List **listptr)
 /*
  * check_subplans_for_ungrouped_vars
  *             Check for subplans that are being passed ungrouped variables as
- *             parameters; return TRUE if any are found.
+ *             parameters; generate an error message if any are found.
  *
  * In most contexts, ungrouped variables will be detected by the parser (see
- * parse_agg.c, exprIsAggOrGroupCol()). But that routine currently does not
- * check subplans, because the necessary info is not computed until the
+ * parse_agg.c, check_ungrouped_columns()). But that routine currently does
+ * not check subplans, because the necessary info is not computed until the
  * planner runs.  So we do it here, after we have processed the subplan.
  * This ought to be cleaned up someday.
  *
  * 'clause' is the expression tree to be searched for subplans.
- * 'groupClause' is the GROUP BY list (a list of GroupClause nodes).
+ * 'query' provides the GROUP BY list and range table.
  * 'targetList' is the target list that the group clauses refer to.
+ * (Is it really necessary to pass the tlist separately?  Couldn't we
+ * just use the tlist found in the query node?)
  */
-bool
+void
 check_subplans_for_ungrouped_vars(Node *clause,
-                                                                 List *groupClause,
+                                                                 Query *query,
                                                                  List *targetList)
 {
        check_subplans_for_ungrouped_vars_context context;
 
-       context.groupClause = groupClause;
+       context.query = query;
        context.targetList = targetList;
-       return check_subplans_for_ungrouped_vars_walker(clause, &context);
+       check_subplans_for_ungrouped_vars_walker(clause, &context);
 }
 
 static bool
@@ -472,10 +475,27 @@ check_subplans_for_ungrouped_vars_walker(Node *node,
                foreach(t, ((Expr *) node)->args)
                {
                        Node       *thisarg = lfirst(t);
-                       bool            contained_in_group_clause = false;
+                       Var                *var;
+                       bool            contained_in_group_clause;
                        List       *gl;
 
-                       foreach(gl, context->groupClause)
+                       /*
+                        * We do not care about args that are not local variables;
+                        * params or outer-level vars are not our responsibility to
+                        * check.  (The outer-level query passing them to us needs
+                        * to worry, instead.)
+                        */
+                       if (! IsA(thisarg, Var))
+                               continue;
+                       var = (Var *) thisarg;
+                       if (var->varlevelsup > 0)
+                               continue;
+
+                       /*
+                        * Else, see if it is a grouping column.
+                        */
+                       contained_in_group_clause = false;
+                       foreach(gl, context->query->groupClause)
                        {
                                GroupClause        *gcl = lfirst(gl);
                                Node               *groupexpr;
@@ -490,7 +510,21 @@ check_subplans_for_ungrouped_vars_walker(Node *node,
                        }
 
                        if (!contained_in_group_clause)
-                               return true;    /* found an ungrouped argument */
+                       {
+                               /* Found an ungrouped argument.  Complain. */
+                               RangeTblEntry  *rte;
+                               char               *attname;
+
+                               Assert(var->varno > 0 &&
+                                          var->varno <= length(context->query->rtable));
+                               rte = rt_fetch(var->varno, context->query->rtable);
+                               attname = get_attname(rte->relid, var->varattno);
+                               if (! attname)
+                                       elog(ERROR, "cache lookup of attribute %d in relation %u failed",
+                                                var->varattno, rte->relid);
+                               elog(ERROR, "Sub-SELECT uses un-GROUPed attribute %s.%s from outer query",
+                                        rte->refname, attname);
+                       }
                }
        }
        return expression_tree_walker(node,
index 5c87d5bc850f961bead62e4a7f237b64170335b2..4e30e60ca65a7b01b11ae5c2fa69c0066f7e1674 100644 (file)
@@ -7,7 +7,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/parser/parse_agg.c,v 1.29 1999/10/07 04:23:12 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/parser/parse_agg.c,v 1.30 1999/12/09 05:58:54 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 #include "parser/parse_agg.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_expr.h"
+#include "parser/parsetree.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
 
+typedef struct {
+       ParseState *pstate;
+       List       *groupClauses;
+} check_ungrouped_columns_context;
+
 static bool contain_agg_clause(Node *clause);
 static bool contain_agg_clause_walker(Node *node, void *context);
-static bool exprIsAggOrGroupCol(Node *expr, List *groupClauses);
-static bool exprIsAggOrGroupCol_walker(Node *node, List *groupClauses);
+static void check_ungrouped_columns(Node *node, ParseState *pstate,
+                                                                       List *groupClauses);
+static bool check_ungrouped_columns_walker(Node *node,
+                                                                                  check_ungrouped_columns_context *context);
 
 /*
  * contain_agg_clause
@@ -53,9 +61,11 @@ contain_agg_clause_walker(Node *node, void *context)
 }
 
 /*
- * exprIsAggOrGroupCol -
- *       returns true if the expression does not contain non-group columns,
- *       other than within the arguments of aggregate functions.
+ * check_ungrouped_columns -
+ *       Scan the given expression tree for ungrouped variables (variables
+ *       that are not listed in the groupClauses list and are not within
+ *       the arguments of aggregate functions).  Emit a suitable error message
+ *       if any are found.
  *
  * NOTE: we assume that the given clause has been transformed suitably for
  * parser output.  This means we can use the planner's expression_tree_walker.
@@ -68,50 +78,70 @@ contain_agg_clause_walker(Node *node, void *context)
  * inside the subquery and converted them into a list of parameters for the
  * subquery.
  */
-static bool
-exprIsAggOrGroupCol(Node *expr, List *groupClauses)
+static void
+check_ungrouped_columns(Node *node, ParseState *pstate,
+                                               List *groupClauses)
 {
-       /* My walker returns TRUE if it finds a subexpression that is NOT
-        * acceptable (since we can abort the recursion at that point).
-        * So, invert its result.
-        */
-       return ! exprIsAggOrGroupCol_walker(expr, groupClauses);
+       check_ungrouped_columns_context context;
+
+       context.pstate = pstate;
+       context.groupClauses = groupClauses;
+       check_ungrouped_columns_walker(node, &context);
 }
 
 static bool
-exprIsAggOrGroupCol_walker(Node *node, List *groupClauses)
+check_ungrouped_columns_walker(Node *node,
+                                                          check_ungrouped_columns_context *context)
 {
        List       *gl;
 
        if (node == NULL)
                return false;
-       if (IsA(node, Aggref))
-               return false;                   /* OK; do not examine argument of aggregate */
        if (IsA(node, Const) || IsA(node, Param))
                return false;                   /* constants are always acceptable */
-       /* Now check to see if expression as a whole matches any GROUP BY item.
+       /*
+        * If we find an aggregate function, do not recurse into its arguments.
+        */
+       if (IsA(node, Aggref))
+               return false;
+       /*
+        * Check to see if subexpression as a whole matches any GROUP BY item.
         * We need to do this at every recursion level so that we recognize
-        * GROUPed-BY expressions.
+        * GROUPed-BY expressions before reaching variables within them.
         */
-       foreach(gl, groupClauses)
+       foreach(gl, context->groupClauses)
        {
                if (equal(node, lfirst(gl)))
                        return false;           /* acceptable, do not descend more */
        }
-       /* If we have an ungrouped Var, we have a failure --- unless it is an
+       /*
+        * If we have an ungrouped Var, we have a failure --- unless it is an
         * outer-level Var.  In that case it's a constant as far as this query
         * level is concerned, and we can accept it.  (If it's ungrouped as far
         * as the upper query is concerned, that's someone else's problem...)
         */
        if (IsA(node, Var))
        {
-               if (((Var *) node)->varlevelsup == 0)
-                       return true;            /* found an ungrouped local variable */
-               return false;                   /* outer-level Var is acceptable */
+               Var                        *var = (Var *) node;
+               RangeTblEntry  *rte;
+               char               *attname;
+
+               if (var->varlevelsup > 0)
+                       return false;           /* outer-level Var is acceptable */
+               /* Found an ungrouped local variable; generate error message */
+               Assert(var->varno > 0 &&
+                          var->varno <= length(context->pstate->p_rtable));
+               rte = rt_fetch(var->varno, context->pstate->p_rtable);
+               attname = get_attname(rte->relid, var->varattno);
+               if (! attname)
+                       elog(ERROR, "cache lookup of attribute %d in relation %u failed",
+                                var->varattno, rte->relid);
+               elog(ERROR, "Attribute %s.%s must be GROUPed or used in an aggregate function",
+                        rte->refname, attname);
        }
        /* Otherwise, recurse. */
-       return expression_tree_walker(node, exprIsAggOrGroupCol_walker,
-                                                                 (void *) groupClauses);
+       return expression_tree_walker(node, check_ungrouped_columns_walker,
+                                                                 (void *) context);
 }
 
 /*
@@ -135,9 +165,9 @@ parseCheckAggregates(ParseState *pstate, Query *qry)
        /*
         * Aggregates must never appear in WHERE clauses. (Note this check
         * should appear first to deliver an appropriate error message;
-        * otherwise we are likely to generate the generic "illegal use of
-        * aggregates in target list" message, which is outright misleading if
-        * the problem is in WHERE.)
+        * otherwise we are likely to complain about some innocent variable
+        * in the target list, which is outright misleading if the problem
+        * is in WHERE.)
         */
        if (contain_agg_clause(qry->qual))
                elog(ERROR, "Aggregates not allowed in WHERE clause");
@@ -146,8 +176,8 @@ parseCheckAggregates(ParseState *pstate, Query *qry)
         * No aggregates allowed in GROUP BY clauses, either.
         *
         * While we are at it, build a list of the acceptable GROUP BY expressions
-        * for use by exprIsAggOrGroupCol() (this avoids repeated scans of the
-        * targetlist within the recursive routines...)
+        * for use by check_ungrouped_columns() (this avoids repeated scans of the
+        * targetlist within the recursive routine...)
         */
        foreach(tl, qry->groupClause)
        {
@@ -161,26 +191,10 @@ parseCheckAggregates(ParseState *pstate, Query *qry)
        }
 
        /*
-        * The expression specified in the HAVING clause can only contain
-        * aggregates, group columns and functions thereof.  As with WHERE,
-        * we want to point the finger at HAVING before the target list.
+        * Check the targetlist and HAVING clause for ungrouped variables.
         */
-       if (!exprIsAggOrGroupCol(qry->havingQual, groupClauses))
-               elog(ERROR,
-                        "Illegal use of aggregates or non-group column in HAVING clause");
-
-       /*
-        * The target list can only contain aggregates, group columns and
-        * functions thereof.
-        */
-       foreach(tl, qry->targetList)
-       {
-               TargetEntry *tle = lfirst(tl);
-
-               if (!exprIsAggOrGroupCol(tle->expr, groupClauses))
-                       elog(ERROR,
-                                "Illegal use of aggregates or non-group column in target list");
-       }
+       check_ungrouped_columns((Node *) qry->targetList, pstate, groupClauses);
+       check_ungrouped_columns((Node *) qry->havingQual, pstate, groupClauses);
 
        /* Release the list storage (but not the pointed-to expressions!) */
        freeList(groupClauses);
index aad158c890435f8b61d86166a5d27a7531771c77..829bf434e787fcbb1f996d89f7750ba2ecd6e3b1 100644 (file)
@@ -6,7 +6,7 @@
  *
  * Copyright (c) 1994, Regents of the University of California
  *
- * $Id: clauses.h,v 1.30 1999/09/26 02:28:44 tgl Exp $
+ * $Id: clauses.h,v 1.31 1999/12/09 05:58:55 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -39,8 +39,8 @@ extern List *make_ands_implicit(Expr *clause);
 
 extern List *pull_constant_clauses(List *quals, List **constantQual);
 extern List *pull_agg_clause(Node *clause);
-extern bool check_subplans_for_ungrouped_vars(Node *clause,
-                                                                                         List *groupClause,
+extern void check_subplans_for_ungrouped_vars(Node *clause,
+                                                                                         Query *query,
                                                                                          List *targetList);
 
 extern void clause_get_relids_vars(Node *clause, Relids *relids, List **vars);
index a9b94c9b1e19270724604427b24c3ffed9f6814b..427198fb4adbd596fb4f54452b614fdd840dfbe0 100644 (file)
@@ -32,7 +32,7 @@ count
 (6 rows)
 
 QUERY: SELECT count(*) FROM test_missing_target GROUP BY a ORDER BY b;
-ERROR:  Illegal use of aggregates or non-group column in target list
+ERROR:  Attribute test_missing_target.b must be GROUPed or used in an aggregate function
 QUERY: SELECT count(*) FROM test_missing_target GROUP BY b ORDER BY b;
 count
 -----
@@ -194,7 +194,7 @@ count
 (4 rows)
 
 QUERY: SELECT count(a) FROM test_missing_target GROUP BY a ORDER BY b;
-ERROR:  Illegal use of aggregates or non-group column in target list
+ERROR:  Attribute test_missing_target.b must be GROUPed or used in an aggregate function
 QUERY: SELECT count(b) FROM test_missing_target GROUP BY b/2 ORDER BY b/2;
 count
 -----