Fix parse_agg.c to detect ungrouped Vars in sub-SELECTs; remove code
authorTom Lane
Fri, 17 Jan 2003 03:25:04 +0000 (03:25 +0000)
committerTom Lane
Fri, 17 Jan 2003 03:25:04 +0000 (03:25 +0000)
that used to do it in planner.  That was an ancient kluge that was
never satisfactory; errors should be detected at parse time when possible.
But at the time we didn't have the support mechanism (expression_tree_walker
et al) to make it convenient to do in the parser.

src/backend/optimizer/plan/planner.c
src/backend/optimizer/util/clauses.c
src/backend/parser/analyze.c
src/backend/parser/parse_agg.c
src/include/optimizer/clauses.h
src/include/parser/parse_agg.h

index 4cf6a09acfecbcc1b117195d871ba66aaffaf187..cd5d266e07adb9466f543700556047b00312d2e2 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v 1.139 2003/01/15 19:35:40 tgl Exp $
+ *   $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v 1.140 2003/01/17 03:25:03 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -208,17 +208,6 @@ subquery_planner(Query *parse, double tuple_fraction)
        /* These are not targetlist items, but close enough... */
    }
 
-   /*
-    * Check for ungrouped variables passed to subplans in targetlist and
-    * HAVING clause (but not in WHERE or JOIN/ON clauses, since those are
-    * evaluated before grouping).  We can't do this any earlier because
-    * we must use the preprocessed targetlist for comparisons of grouped
-    * expressions.
-    */
-   if (parse->hasSubLinks &&
-       (parse->groupClause != NIL || parse->hasAggs))
-       check_subplans_for_ungrouped_vars(parse);
-
    /*
     * A HAVING clause without aggregates is equivalent to a WHERE clause
     * (except it can only refer to grouped fields).  Transfer any
index d9bdfec54e2cb69852c5bdd1a911a1a4263a9ac4..acd17ba87d29817de0d78bbbdb37694a6ff0e679 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.123 2003/01/17 02:01:16 tgl Exp $
+ *   $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.124 2003/01/17 03:25:03 tgl Exp $
  *
  * HISTORY
  *   AUTHOR            DATE            MAJOR EVENT
@@ -20,7 +20,6 @@
 #include "postgres.h"
 
 #include "catalog/pg_language.h"
-#include "catalog/pg_operator.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
 #include "executor/executor.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/clauses.h"
-#include "optimizer/tlist.h"
 #include "optimizer/var.h"
 #include "parser/analyze.h"
-#include "parser/parsetree.h"
 #include "tcop/tcopprot.h"
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #define MAKEBOOLCONST(val,isnull) \
    ((Node *) makeConst(BOOLOID, 1, (Datum) (val), (isnull), true))
 
-typedef struct
-{
-   Query      *query;
-   List       *groupClauses;
-} check_subplans_for_ungrouped_vars_context;
-
 typedef struct
 {
    int         nargs;
@@ -64,8 +55,6 @@ static bool pull_agg_clause_walker(Node *node, List **listptr);
 static bool expression_returns_set_walker(Node *node, void *context);
 static bool contain_subplans_walker(Node *node, void *context);
 static bool pull_subplans_walker(Node *node, List **listptr);
-static bool check_subplans_for_ungrouped_vars_walker(Node *node,
-                    check_subplans_for_ungrouped_vars_context *context);
 static bool contain_mutable_functions_walker(Node *node, void *context);
 static bool contain_volatile_functions_walker(Node *node, void *context);
 static bool contain_nonstrict_functions_walker(Node *node, void *context);
@@ -552,157 +541,6 @@ pull_subplans_walker(Node *node, List **listptr)
                                  (void *) listptr);
 }
 
-/*
- * check_subplans_for_ungrouped_vars
- *     Check for subplans that are being passed ungrouped variables as
- *     parameters; generate an error message if any are found.
- *
- * In most contexts, ungrouped variables will be detected by the parser (see
- * 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 sublinks into
- * subplans.  This ought to be cleaned up someday.
- *
- * A deficiency in this scheme is that any outer reference var must be
- * grouped by itself; we don't recognize groupable expressions within
- * subselects. For example, consider
- *     SELECT
- *         (SELECT x FROM bar where y = (foo.a + foo.b))
- *     FROM foo
- *     GROUP BY a + b;
- * This query will be rejected although it could be allowed.
- */
-void
-check_subplans_for_ungrouped_vars(Query *query)
-{
-   check_subplans_for_ungrouped_vars_context context;
-   List       *gl;
-
-   context.query = query;
-
-   /*
-    * Build a list of the acceptable GROUP BY expressions for use in the
-    * walker (to avoid repeated scans of the targetlist within the
-    * recursive routine).
-    */
-   context.groupClauses = NIL;
-   foreach(gl, query->groupClause)
-   {
-       GroupClause *grpcl = lfirst(gl);
-       Node       *expr;
-
-       expr = get_sortgroupclause_expr(grpcl, query->targetList);
-       context.groupClauses = lcons(expr, context.groupClauses);
-   }
-
-   /*
-    * Recursively scan the targetlist and the HAVING clause. WHERE and
-    * JOIN/ON conditions are not examined, since they are evaluated
-    * before grouping.
-    */
-   check_subplans_for_ungrouped_vars_walker((Node *) query->targetList,
-                                            &context);
-   check_subplans_for_ungrouped_vars_walker(query->havingQual,
-                                            &context);
-
-   freeList(context.groupClauses);
-}
-
-static bool
-check_subplans_for_ungrouped_vars_walker(Node *node,
-                     check_subplans_for_ungrouped_vars_context *context)
-{
-   List       *gl;
-
-   if (node == NULL)
-       return false;
-   if (IsA(node, Const) ||
-       IsA(node, Param))
-       return false;           /* constants are always acceptable */
-
-   /*
-    * If we find an aggregate function, do not recurse into its
-    * arguments.  Subplans invoked within aggregate calls are allowed to
-    * receive ungrouped variables.  (This test and the next one should
-    * match the logic in parse_agg.c's check_ungrouped_columns().)
-    */
-   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 before reaching variables within them.
-    */
-   foreach(gl, context->groupClauses)
-   {
-       if (equal(node, lfirst(gl)))
-           return false;       /* acceptable, do not descend more */
-   }
-
-   /*
-    * We can ignore Vars other than in subplan args lists, since the
-    * parser already checked 'em.
-    */
-   if (is_subplan(node))
-   {
-       /*
-        * The args list of the subplan node represents attributes from
-        * outside passed into the sublink.
-        */
-       List       *t;
-
-       foreach(t, ((SubPlan *) node)->args)
-       {
-           Node       *thisarg = lfirst(t);
-           Var        *var;
-           bool        contained_in_group_clause;
-
-           /*
-            * 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->groupClauses)
-           {
-               if (equal(thisarg, lfirst(gl)))
-               {
-                   contained_in_group_clause = true;
-                   break;
-               }
-           }
-
-           if (!contained_in_group_clause)
-           {
-               /* Found an ungrouped argument.  Complain. */
-               RangeTblEntry *rte;
-               char       *attname;
-
-               Assert(var->varno > 0 &&
-                    (int) var->varno <= length(context->query->rtable));
-               rte = rt_fetch(var->varno, context->query->rtable);
-               attname = get_rte_attribute_name(rte, var->varattno);
-               elog(ERROR, "Sub-SELECT uses un-GROUPed attribute %s.%s from outer query",
-                    rte->eref->aliasname, attname);
-           }
-       }
-   }
-   return expression_tree_walker(node,
-                               check_subplans_for_ungrouped_vars_walker,
-                                 (void *) context);
-}
-
 
 /*****************************************************************************
  *     Check clauses for mutable functions
index 2609de18f54656f16a85c3175570fe01fc5c27bc..175059b96cff6f4d73f77ba73779a3e000d14c98 100644 (file)
@@ -6,7 +6,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Header: /cvsroot/pgsql/src/backend/parser/analyze.c,v 1.259 2003/01/02 19:29:22 tgl Exp $
+ * $Header: /cvsroot/pgsql/src/backend/parser/analyze.c,v 1.260 2003/01/17 03:25:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -354,7 +354,7 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt)
    qry->hasSubLinks = pstate->p_hasSubLinks;
    qry->hasAggs = pstate->p_hasAggs;
    if (pstate->p_hasAggs)
-       parseCheckAggregates(pstate, qry, qual);
+       parseCheckAggregates(pstate, qry);
 
    return qry;
 }
@@ -575,7 +575,7 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt,
    qry->hasSubLinks = pstate->p_hasSubLinks;
    qry->hasAggs = pstate->p_hasAggs;
    if (pstate->p_hasAggs)
-       parseCheckAggregates(pstate, qry, NULL);
+       parseCheckAggregates(pstate, qry);
 
    return qry;
 }
@@ -1671,13 +1671,13 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
    qry->limitOffset = stmt->limitOffset;
    qry->limitCount = stmt->limitCount;
 
+   qry->rtable = pstate->p_rtable;
+   qry->jointree = makeFromExpr(pstate->p_joinlist, qual);
+
    qry->hasSubLinks = pstate->p_hasSubLinks;
    qry->hasAggs = pstate->p_hasAggs;
    if (pstate->p_hasAggs || qry->groupClause || qry->havingQual)
-       parseCheckAggregates(pstate, qry, qual);
-
-   qry->rtable = pstate->p_rtable;
-   qry->jointree = makeFromExpr(pstate->p_joinlist, qual);
+       parseCheckAggregates(pstate, qry);
 
    if (stmt->forUpdate != NIL)
        transformForUpdate(qry, stmt->forUpdate);
@@ -1900,13 +1900,13 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt)
    qry->limitOffset = limitOffset;
    qry->limitCount = limitCount;
 
+   qry->rtable = pstate->p_rtable;
+   qry->jointree = makeFromExpr(pstate->p_joinlist, NULL);
+
    qry->hasSubLinks = pstate->p_hasSubLinks;
    qry->hasAggs = pstate->p_hasAggs;
    if (pstate->p_hasAggs || qry->groupClause || qry->havingQual)
-       parseCheckAggregates(pstate, qry, NULL);
-
-   qry->rtable = pstate->p_rtable;
-   qry->jointree = makeFromExpr(pstate->p_joinlist, NULL);
+       parseCheckAggregates(pstate, qry);
 
    if (forUpdate != NIL)
        transformForUpdate(qry, forUpdate);
@@ -2146,7 +2146,7 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt)
    qry->hasSubLinks = pstate->p_hasSubLinks;
    qry->hasAggs = pstate->p_hasAggs;
    if (pstate->p_hasAggs)
-       parseCheckAggregates(pstate, qry, qual);
+       parseCheckAggregates(pstate, qry);
 
    /*
     * Now we are done with SELECT-like processing, and can get on with
index 127abdc2de8567fa2aeeb93a2d29eab385f52acf..f980d52f2b0e72e2fbee05a4f094227dcc2aa367 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $Header: /cvsroot/pgsql/src/backend/parser/parse_agg.c,v 1.50 2002/06/20 20:29:32 momjian Exp $
+ *   $Header: /cvsroot/pgsql/src/backend/parser/parse_agg.c,v 1.51 2003/01/17 03:25:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -24,10 +24,12 @@ typedef struct
 {
    ParseState *pstate;
    List       *groupClauses;
+   bool        have_non_var_grouping;
+   int         sublevels_up;
 } check_ungrouped_columns_context;
 
 static void check_ungrouped_columns(Node *node, ParseState *pstate,
-                       List *groupClauses);
+                       List *groupClauses, bool have_non_var_grouping);
 static bool check_ungrouped_columns_walker(Node *node,
                               check_ungrouped_columns_context *context);
 
@@ -39,24 +41,29 @@ static bool check_ungrouped_columns_walker(Node *node,
  *   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.
+ * parser output.  This means we can use expression_tree_walker.
  *
- * NOTE: in the case of a SubLink, expression_tree_walker does not descend
- * into the subquery.  This means we will fail to detect ungrouped columns
- * that appear as outer-level variables within a subquery. That case seems
- * unreasonably hard to handle here.  Instead, we expect the planner to check
- * for ungrouped columns after it's found all the outer-level references
- * inside the subquery and converted them into a list of parameters for the
- * subquery.
+ * NOTE: we recognize grouping expressions in the main query, but only
+ * grouping Vars in subqueries.  For example, this will be rejected,
+ * although it could be allowed:
+ *     SELECT
+ *         (SELECT x FROM bar where y = (foo.a + foo.b))
+ *     FROM foo
+ *     GROUP BY a + b;
+ * The difficulty is the need to account for different sublevels_up.
+ * This appears to require a whole custom version of equal(), which is
+ * way more pain than the feature seems worth.
  */
 static void
 check_ungrouped_columns(Node *node, ParseState *pstate,
-                       List *groupClauses)
+                       List *groupClauses, bool have_non_var_grouping)
 {
    check_ungrouped_columns_context context;
 
    context.pstate = pstate;
    context.groupClauses = groupClauses;
+   context.have_non_var_grouping = have_non_var_grouping;
+   context.sublevels_up = 0;
    check_ungrouped_columns_walker(node, &context);
 }
 
@@ -68,32 +75,38 @@ check_ungrouped_columns_walker(Node *node,
 
    if (node == NULL)
        return false;
-   if (IsA(node, Const) ||IsA(node, Param))
+   if (IsA(node, Const) ||
+       IsA(node, Param))
        return false;           /* constants are always acceptable */
 
    /*
     * If we find an aggregate function, do not recurse into its
-    * arguments.
+    * arguments; ungrouped vars in the arguments are not an error.
     */
    if (IsA(node, Aggref))
        return false;
 
    /*
-    * Check to see if subexpression as a whole matches any GROUP BY item.
+    * If we have any GROUP BY items that are not simple Vars,
+    * 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 before reaching variables within them.
+    * But this only works at the outer query level, as noted above.
     */
-   foreach(gl, context->groupClauses)
+   if (context->have_non_var_grouping && context->sublevels_up == 0)
    {
-       if (equal(node, lfirst(gl)))
-           return false;       /* acceptable, do not descend more */
+       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
-    * 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
+    * If we have an ungrouped Var of the original query level, we have a
+    * failure.  Vars below the original query level are not a problem,
+    * and neither are Vars from above it.  (If such Vars are ungrouped as
+    * far as their own query level is concerned, that's someone else's
     * problem...)
     */
    if (IsA(node, Var))
@@ -102,17 +115,52 @@ check_ungrouped_columns_walker(Node *node,
        RangeTblEntry *rte;
        char       *attname;
 
-       if (var->varlevelsup > 0)
-           return false;       /* outer-level Var is acceptable */
+       if (var->varlevelsup != context->sublevels_up)
+           return false;       /* it's not local to my query, ignore */
+       /*
+        * Check for a match, if we didn't do it above.
+        */
+       if (!context->have_non_var_grouping || context->sublevels_up != 0)
+       {
+           foreach(gl, context->groupClauses)
+           {
+               Var    *gvar = (Var *) lfirst(gl);
+
+               if (IsA(gvar, Var) &&
+                   gvar->varno == var->varno &&
+                   gvar->varattno == var->varattno &&
+                   gvar->varlevelsup == 0)
+                   return false; /* acceptable, we're okay */
+           }
+       }
+
        /* Found an ungrouped local variable; generate error message */
        Assert(var->varno > 0 &&
               (int) var->varno <= length(context->pstate->p_rtable));
        rte = rt_fetch(var->varno, context->pstate->p_rtable);
        attname = get_rte_attribute_name(rte, var->varattno);
-       elog(ERROR, "Attribute %s.%s must be GROUPed or used in an aggregate function",
-            rte->eref->aliasname, attname);
+       if (context->sublevels_up == 0)
+           elog(ERROR, "Attribute %s.%s must be GROUPed or used in an aggregate function",
+                rte->eref->aliasname, attname);
+       else
+           elog(ERROR, "Sub-SELECT uses un-GROUPed attribute %s.%s from outer query",
+                rte->eref->aliasname, attname);
+
+   }
+
+   if (IsA(node, Query))
+   {
+       /* Recurse into subselects */
+       bool        result;
+
+       context->sublevels_up++;
+       result = query_tree_walker((Query *) node,
+                                  check_ungrouped_columns_walker,
+                                  (void *) context,
+                                  0);
+       context->sublevels_up--;
+       return result;
    }
-   /* Otherwise, recurse. */
    return expression_tree_walker(node, check_ungrouped_columns_walker,
                                  (void *) context);
 }
@@ -124,15 +172,13 @@ check_ungrouped_columns_walker(Node *node,
  * Ideally this should be done earlier, but it's difficult to distinguish
  * aggregates from plain functions at the grammar level.  So instead we
  * check here.  This function should be called after the target list and
- * qualifications are finalized.  BUT: in some cases we want to call this
- * routine before we've assembled the joinlist and qual into a FromExpr.
- * So, rather than looking at qry->jointree, look at pstate->p_joinlist
- * and the explicitly-passed qual.
+ * qualifications are finalized.
  */
 void
-parseCheckAggregates(ParseState *pstate, Query *qry, Node *qual)
+parseCheckAggregates(ParseState *pstate, Query *qry)
 {
    List       *groupClauses = NIL;
+   bool        have_non_var_grouping = false;
    List       *tl;
 
    /* This should only be called if we found aggregates, GROUP, or HAVING */
@@ -146,9 +192,9 @@ parseCheckAggregates(ParseState *pstate, Query *qry, Node *qual)
     * variable in the target list, which is outright misleading if the
     * problem is in WHERE.)
     */
-   if (contain_agg_clause(qual))
+   if (contain_agg_clause(qry->jointree->quals))
        elog(ERROR, "Aggregates not allowed in WHERE clause");
-   if (contain_agg_clause((Node *) pstate->p_joinlist))
+   if (contain_agg_clause((Node *) qry->jointree->fromlist))
        elog(ERROR, "Aggregates not allowed in JOIN conditions");
 
    /*
@@ -156,7 +202,8 @@ parseCheckAggregates(ParseState *pstate, Query *qry, Node *qual)
     *
     * While we are at it, build a list of the acceptable GROUP BY
     * expressions for use by check_ungrouped_columns() (this avoids
-    * repeated scans of the targetlist within the recursive routine...)
+    * repeated scans of the targetlist within the recursive routine...).
+    * And detect whether any of the expressions aren't simple Vars.
     */
    foreach(tl, qry->groupClause)
    {
@@ -164,16 +211,22 @@ parseCheckAggregates(ParseState *pstate, Query *qry, Node *qual)
        Node       *expr;
 
        expr = get_sortgroupclause_expr(grpcl, qry->targetList);
+       if (expr == NULL)
+           continue;           /* probably cannot happen */
        if (contain_agg_clause(expr))
            elog(ERROR, "Aggregates not allowed in GROUP BY clause");
        groupClauses = lcons(expr, groupClauses);
+       if (!IsA(expr, Var))
+           have_non_var_grouping = true;
    }
 
    /*
     * Check the targetlist and HAVING clause for ungrouped variables.
     */
-   check_ungrouped_columns((Node *) qry->targetList, pstate, groupClauses);
-   check_ungrouped_columns((Node *) qry->havingQual, pstate, groupClauses);
+   check_ungrouped_columns((Node *) qry->targetList, pstate,
+                           groupClauses, have_non_var_grouping);
+   check_ungrouped_columns((Node *) qry->havingQual, pstate,
+                           groupClauses, have_non_var_grouping);
 
    /* Release the list storage (but not the pointed-to expressions!) */
    freeList(groupClauses);
index 024c1599a4c77d08dd158138e851a5e4eec10f1c..7ee0f1be2e4b71bcfeb06e3482ea762bf65d55cb 100644 (file)
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: clauses.h,v 1.60 2003/01/17 02:01:21 tgl Exp $
+ * $Id: clauses.h,v 1.61 2003/01/17 03:25:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -52,7 +52,6 @@ extern bool expression_returns_set(Node *clause);
 
 extern bool contain_subplans(Node *clause);
 extern List *pull_subplans(Node *clause);
-extern void check_subplans_for_ungrouped_vars(Query *query);
 
 extern bool contain_mutable_functions(Node *clause);
 extern bool contain_volatile_functions(Node *clause);
index e836310f82ec527e0ff60952aaa9691e66ff0ca0..111d726bc9c3b8e69ea98afb7ca99ae9d1a642f3 100644 (file)
@@ -1,13 +1,12 @@
 /*-------------------------------------------------------------------------
  *
  * parse_agg.h
- *
- *
+ *   handle aggregates in parser
  *
  * Portions Copyright (c) 1996-2002, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: parse_agg.h,v 1.24 2002/06/20 20:29:51 momjian Exp $
+ * $Id: parse_agg.h,v 1.25 2003/01/17 03:25:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -16,6 +15,6 @@
 
 #include "parser/parse_node.h"
 
-extern void parseCheckAggregates(ParseState *pstate, Query *qry, Node *qual);
+extern void parseCheckAggregates(ParseState *pstate, Query *qry);
 
 #endif   /* PARSE_AGG_H */