Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit f7f41c7

Browse files
committed
Replace generic 'Illegal use of aggregates' error message with one that
shows the specific ungrouped variable being complained of. Perhaps this will reduce user confusion...
1 parent d65a27f commit f7f41c7

File tree

6 files changed

+121
-77
lines changed

6 files changed

+121
-77
lines changed

src/backend/optimizer/plan/planmain.c

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*
88
*
99
* IDENTIFICATION
10-
* $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planmain.c,v 1.47 1999/11/15 02:00:07 tgl Exp $
10+
* $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planmain.c,v 1.48 1999/12/09 05:58:52 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -100,10 +100,7 @@ query_planner(Query *root,
100100
* Note we do NOT do this for subplans in WHERE; it's legal
101101
* there because WHERE is evaluated pre-GROUP.
102102
*/
103-
if (check_subplans_for_ungrouped_vars((Node *) tlist,
104-
root->groupClause,
105-
tlist))
106-
elog(ERROR, "Sub-SELECT must use only GROUPed attributes from outer SELECT");
103+
check_subplans_for_ungrouped_vars((Node *) tlist, root, tlist);
107104
}
108105
}
109106

src/backend/optimizer/plan/planner.c

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*
88
*
99
* IDENTIFICATION
10-
* $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v 1.71 1999/11/15 02:00:08 tgl Exp $
10+
* $Header: /cvsroot/pgsql/src/backend/optimizer/plan/planner.c,v 1.72 1999/12/09 05:58:52 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -344,10 +344,9 @@ union_planner(Query *parse)
344344
/* Expand SubLinks to SubPlans */
345345
parse->havingQual = SS_process_sublinks(parse->havingQual);
346346
/* Check for ungrouped variables passed to subplans */
347-
if (check_subplans_for_ungrouped_vars(parse->havingQual,
348-
parse->groupClause,
349-
parse->targetList))
350-
elog(ERROR, "Sub-SELECT must use only GROUPed attributes from outer SELECT");
347+
check_subplans_for_ungrouped_vars(parse->havingQual,
348+
parse,
349+
parse->targetList);
351350
}
352351
}
353352

src/backend/optimizer/util/clauses.c

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*
88
*
99
* IDENTIFICATION
10-
* $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.55 1999/11/22 17:56:17 momjian Exp $
10+
* $Header: /cvsroot/pgsql/src/backend/optimizer/util/clauses.c,v 1.56 1999/12/09 05:58:53 tgl Exp $
1111
*
1212
* HISTORY
1313
* AUTHOR DATE MAJOR EVENT
@@ -30,6 +30,7 @@
3030
#include "optimizer/tlist.h"
3131
#include "optimizer/var.h"
3232
#include "parser/parse_type.h"
33+
#include "parser/parsetree.h"
3334
#include "utils/lsyscache.h"
3435
#include "utils/syscache.h"
3536

@@ -40,7 +41,7 @@
4041
(isnull), true, false, false))
4142

4243
typedef struct {
43-
List *groupClause;
44+
Query *query;
4445
List *targetList;
4546
} check_subplans_for_ungrouped_vars_context;
4647

@@ -427,28 +428,30 @@ pull_agg_clause_walker(Node *node, List **listptr)
427428
/*
428429
* check_subplans_for_ungrouped_vars
429430
* Check for subplans that are being passed ungrouped variables as
430-
* parameters; return TRUE if any are found.
431+
* parameters; generate an error message if any are found.
431432
*
432433
* In most contexts, ungrouped variables will be detected by the parser (see
433-
* parse_agg.c, exprIsAggOrGroupCol()). But that routine currently does not
434-
* check subplans, because the necessary info is not computed until the
434+
* parse_agg.c, check_ungrouped_columns()). But that routine currently does
435+
* not check subplans, because the necessary info is not computed until the
435436
* planner runs. So we do it here, after we have processed the subplan.
436437
* This ought to be cleaned up someday.
437438
*
438439
* 'clause' is the expression tree to be searched for subplans.
439-
* 'groupClause' is the GROUP BY list (a list of GroupClause nodes).
440+
* 'query' provides the GROUP BY list and range table.
440441
* 'targetList' is the target list that the group clauses refer to.
442+
* (Is it really necessary to pass the tlist separately? Couldn't we
443+
* just use the tlist found in the query node?)
441444
*/
442-
bool
445+
void
443446
check_subplans_for_ungrouped_vars(Node *clause,
444-
List *groupClause,
447+
Query *query,
445448
List *targetList)
446449
{
447450
check_subplans_for_ungrouped_vars_context context;
448451

449-
context.groupClause = groupClause;
452+
context.query = query;
450453
context.targetList = targetList;
451-
return check_subplans_for_ungrouped_vars_walker(clause, &context);
454+
check_subplans_for_ungrouped_vars_walker(clause, &context);
452455
}
453456

454457
static bool
@@ -472,10 +475,27 @@ check_subplans_for_ungrouped_vars_walker(Node *node,
472475
foreach(t, ((Expr *) node)->args)
473476
{
474477
Node *thisarg = lfirst(t);
475-
bool contained_in_group_clause = false;
478+
Var *var;
479+
bool contained_in_group_clause;
476480
List *gl;
477481

478-
foreach(gl, context->groupClause)
482+
/*
483+
* We do not care about args that are not local variables;
484+
* params or outer-level vars are not our responsibility to
485+
* check. (The outer-level query passing them to us needs
486+
* to worry, instead.)
487+
*/
488+
if (! IsA(thisarg, Var))
489+
continue;
490+
var = (Var *) thisarg;
491+
if (var->varlevelsup > 0)
492+
continue;
493+
494+
/*
495+
* Else, see if it is a grouping column.
496+
*/
497+
contained_in_group_clause = false;
498+
foreach(gl, context->query->groupClause)
479499
{
480500
GroupClause *gcl = lfirst(gl);
481501
Node *groupexpr;
@@ -490,7 +510,21 @@ check_subplans_for_ungrouped_vars_walker(Node *node,
490510
}
491511

492512
if (!contained_in_group_clause)
493-
return true; /* found an ungrouped argument */
513+
{
514+
/* Found an ungrouped argument. Complain. */
515+
RangeTblEntry *rte;
516+
char *attname;
517+
518+
Assert(var->varno > 0 &&
519+
var->varno <= length(context->query->rtable));
520+
rte = rt_fetch(var->varno, context->query->rtable);
521+
attname = get_attname(rte->relid, var->varattno);
522+
if (! attname)
523+
elog(ERROR, "cache lookup of attribute %d in relation %u failed",
524+
var->varattno, rte->relid);
525+
elog(ERROR, "Sub-SELECT uses un-GROUPed attribute %s.%s from outer query",
526+
rte->refname, attname);
527+
}
494528
}
495529
}
496530
return expression_tree_walker(node,

src/backend/parser/parse_agg.c

Lines changed: 63 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
*
88
*
99
* IDENTIFICATION
10-
* $Header: /cvsroot/pgsql/src/backend/parser/parse_agg.c,v 1.29 1999/10/07 04:23:12 tgl Exp $
10+
* $Header: /cvsroot/pgsql/src/backend/parser/parse_agg.c,v 1.30 1999/12/09 05:58:54 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -19,13 +19,21 @@
1919
#include "parser/parse_agg.h"
2020
#include "parser/parse_coerce.h"
2121
#include "parser/parse_expr.h"
22+
#include "parser/parsetree.h"
2223
#include "utils/lsyscache.h"
2324
#include "utils/syscache.h"
2425

26+
typedef struct {
27+
ParseState *pstate;
28+
List *groupClauses;
29+
} check_ungrouped_columns_context;
30+
2531
static bool contain_agg_clause(Node *clause);
2632
static bool contain_agg_clause_walker(Node *node, void *context);
27-
static bool exprIsAggOrGroupCol(Node *expr, List *groupClauses);
28-
static bool exprIsAggOrGroupCol_walker(Node *node, List *groupClauses);
33+
static void check_ungrouped_columns(Node *node, ParseState *pstate,
34+
List *groupClauses);
35+
static bool check_ungrouped_columns_walker(Node *node,
36+
check_ungrouped_columns_context *context);
2937

3038
/*
3139
* contain_agg_clause
@@ -53,9 +61,11 @@ contain_agg_clause_walker(Node *node, void *context)
5361
}
5462

5563
/*
56-
* exprIsAggOrGroupCol -
57-
* returns true if the expression does not contain non-group columns,
58-
* other than within the arguments of aggregate functions.
64+
* check_ungrouped_columns -
65+
* Scan the given expression tree for ungrouped variables (variables
66+
* that are not listed in the groupClauses list and are not within
67+
* the arguments of aggregate functions). Emit a suitable error message
68+
* if any are found.
5969
*
6070
* NOTE: we assume that the given clause has been transformed suitably for
6171
* 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)
6878
* inside the subquery and converted them into a list of parameters for the
6979
* subquery.
7080
*/
71-
static bool
72-
exprIsAggOrGroupCol(Node *expr, List *groupClauses)
81+
static void
82+
check_ungrouped_columns(Node *node, ParseState *pstate,
83+
List *groupClauses)
7384
{
74-
/* My walker returns TRUE if it finds a subexpression that is NOT
75-
* acceptable (since we can abort the recursion at that point).
76-
* So, invert its result.
77-
*/
78-
return ! exprIsAggOrGroupCol_walker(expr, groupClauses);
85+
check_ungrouped_columns_context context;
86+
87+
context.pstate = pstate;
88+
context.groupClauses = groupClauses;
89+
check_ungrouped_columns_walker(node, &context);
7990
}
8091

8192
static bool
82-
exprIsAggOrGroupCol_walker(Node *node, List *groupClauses)
93+
check_ungrouped_columns_walker(Node *node,
94+
check_ungrouped_columns_context *context)
8395
{
8496
List *gl;
8597

8698
if (node == NULL)
8799
return false;
88-
if (IsA(node, Aggref))
89-
return false; /* OK; do not examine argument of aggregate */
90100
if (IsA(node, Const) || IsA(node, Param))
91101
return false; /* constants are always acceptable */
92-
/* Now check to see if expression as a whole matches any GROUP BY item.
102+
/*
103+
* If we find an aggregate function, do not recurse into its arguments.
104+
*/
105+
if (IsA(node, Aggref))
106+
return false;
107+
/*
108+
* Check to see if subexpression as a whole matches any GROUP BY item.
93109
* We need to do this at every recursion level so that we recognize
94-
* GROUPed-BY expressions.
110+
* GROUPed-BY expressions before reaching variables within them.
95111
*/
96-
foreach(gl, groupClauses)
112+
foreach(gl, context->groupClauses)
97113
{
98114
if (equal(node, lfirst(gl)))
99115
return false; /* acceptable, do not descend more */
100116
}
101-
/* If we have an ungrouped Var, we have a failure --- unless it is an
117+
/*
118+
* If we have an ungrouped Var, we have a failure --- unless it is an
102119
* outer-level Var. In that case it's a constant as far as this query
103120
* level is concerned, and we can accept it. (If it's ungrouped as far
104121
* as the upper query is concerned, that's someone else's problem...)
105122
*/
106123
if (IsA(node, Var))
107124
{
108-
if (((Var *) node)->varlevelsup == 0)
109-
return true; /* found an ungrouped local variable */
110-
return false; /* outer-level Var is acceptable */
125+
Var *var = (Var *) node;
126+
RangeTblEntry *rte;
127+
char *attname;
128+
129+
if (var->varlevelsup > 0)
130+
return false; /* outer-level Var is acceptable */
131+
/* Found an ungrouped local variable; generate error message */
132+
Assert(var->varno > 0 &&
133+
var->varno <= length(context->pstate->p_rtable));
134+
rte = rt_fetch(var->varno, context->pstate->p_rtable);
135+
attname = get_attname(rte->relid, var->varattno);
136+
if (! attname)
137+
elog(ERROR, "cache lookup of attribute %d in relation %u failed",
138+
var->varattno, rte->relid);
139+
elog(ERROR, "Attribute %s.%s must be GROUPed or used in an aggregate function",
140+
rte->refname, attname);
111141
}
112142
/* Otherwise, recurse. */
113-
return expression_tree_walker(node, exprIsAggOrGroupCol_walker,
114-
(void *) groupClauses);
143+
return expression_tree_walker(node, check_ungrouped_columns_walker,
144+
(void *) context);
115145
}
116146

117147
/*
@@ -135,9 +165,9 @@ parseCheckAggregates(ParseState *pstate, Query *qry)
135165
/*
136166
* Aggregates must never appear in WHERE clauses. (Note this check
137167
* should appear first to deliver an appropriate error message;
138-
* otherwise we are likely to generate the generic "illegal use of
139-
* aggregates in target list" message, which is outright misleading if
140-
* the problem is in WHERE.)
168+
* otherwise we are likely to complain about some innocent variable
169+
* in the target list, which is outright misleading if the problem
170+
* is in WHERE.)
141171
*/
142172
if (contain_agg_clause(qry->qual))
143173
elog(ERROR, "Aggregates not allowed in WHERE clause");
@@ -146,8 +176,8 @@ parseCheckAggregates(ParseState *pstate, Query *qry)
146176
* No aggregates allowed in GROUP BY clauses, either.
147177
*
148178
* While we are at it, build a list of the acceptable GROUP BY expressions
149-
* for use by exprIsAggOrGroupCol() (this avoids repeated scans of the
150-
* targetlist within the recursive routines...)
179+
* for use by check_ungrouped_columns() (this avoids repeated scans of the
180+
* targetlist within the recursive routine...)
151181
*/
152182
foreach(tl, qry->groupClause)
153183
{
@@ -161,26 +191,10 @@ parseCheckAggregates(ParseState *pstate, Query *qry)
161191
}
162192

163193
/*
164-
* The expression specified in the HAVING clause can only contain
165-
* aggregates, group columns and functions thereof. As with WHERE,
166-
* we want to point the finger at HAVING before the target list.
194+
* Check the targetlist and HAVING clause for ungrouped variables.
167195
*/
168-
if (!exprIsAggOrGroupCol(qry->havingQual, groupClauses))
169-
elog(ERROR,
170-
"Illegal use of aggregates or non-group column in HAVING clause");
171-
172-
/*
173-
* The target list can only contain aggregates, group columns and
174-
* functions thereof.
175-
*/
176-
foreach(tl, qry->targetList)
177-
{
178-
TargetEntry *tle = lfirst(tl);
179-
180-
if (!exprIsAggOrGroupCol(tle->expr, groupClauses))
181-
elog(ERROR,
182-
"Illegal use of aggregates or non-group column in target list");
183-
}
196+
check_ungrouped_columns((Node *) qry->targetList, pstate, groupClauses);
197+
check_ungrouped_columns((Node *) qry->havingQual, pstate, groupClauses);
184198

185199
/* Release the list storage (but not the pointed-to expressions!) */
186200
freeList(groupClauses);

src/include/optimizer/clauses.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
*
77
* Copyright (c) 1994, Regents of the University of California
88
*
9-
* $Id: clauses.h,v 1.30 1999/09/26 02:28:44 tgl Exp $
9+
* $Id: clauses.h,v 1.31 1999/12/09 05:58:55 tgl Exp $
1010
*
1111
*-------------------------------------------------------------------------
1212
*/
@@ -39,8 +39,8 @@ extern List *make_ands_implicit(Expr *clause);
3939

4040
extern List *pull_constant_clauses(List *quals, List **constantQual);
4141
extern List *pull_agg_clause(Node *clause);
42-
extern bool check_subplans_for_ungrouped_vars(Node *clause,
43-
List *groupClause,
42+
extern void check_subplans_for_ungrouped_vars(Node *clause,
43+
Query *query,
4444
List *targetList);
4545

4646
extern void clause_get_relids_vars(Node *clause, Relids *relids, List **vars);

src/test/regress/expected/select_implicit.out

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ count
3232
(6 rows)
3333

3434
QUERY: SELECT count(*) FROM test_missing_target GROUP BY a ORDER BY b;
35-
ERROR: Illegal use of aggregates or non-group column in target list
35+
ERROR: Attribute test_missing_target.b must be GROUPed or used in an aggregate function
3636
QUERY: SELECT count(*) FROM test_missing_target GROUP BY b ORDER BY b;
3737
count
3838
-----
@@ -194,7 +194,7 @@ count
194194
(4 rows)
195195

196196
QUERY: SELECT count(a) FROM test_missing_target GROUP BY a ORDER BY b;
197-
ERROR: Illegal use of aggregates or non-group column in target list
197+
ERROR: Attribute test_missing_target.b must be GROUPed or used in an aggregate function
198198
QUERY: SELECT count(b) FROM test_missing_target GROUP BY b/2 ORDER BY b/2;
199199
count
200200
-----

0 commit comments

Comments
 (0)