diff --git a/src/backend/optimizer/plan/planmain.c b/src/backend/optimizer/plan/planmain.c index 7da2ee4c21..7dcafe35dc 100644 --- a/src/backend/optimizer/plan/planmain.c +++ b/src/backend/optimizer/plan/planmain.c @@ -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); } } diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 295d722b6a..278ef356f1 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -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); } } diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 07ccca7bf0..63b3ff87d9 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -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, diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index 5c87d5bc85..4e30e60ca6 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -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 $ * *------------------------------------------------------------------------- */ @@ -19,13 +19,21 @@ #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. - * We need to do this at every recursion level so that we recognize - * GROUPed-BY expressions. + /* + * If we find an aggregate function, do not recurse into its arguments. */ - foreach(gl, groupClauses) + 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 */ } - /* 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); diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h index aad158c890..829bf434e7 100644 --- a/src/include/optimizer/clauses.h +++ b/src/include/optimizer/clauses.h @@ -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); diff --git a/src/test/regress/expected/select_implicit.out b/src/test/regress/expected/select_implicit.out index a9b94c9b1e..427198fb4a 100644 --- a/src/test/regress/expected/select_implicit.out +++ b/src/test/regress/expected/select_implicit.out @@ -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 -----