Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitf7f41c7

Browse files
committed
Replace generic 'Illegal use of aggregates' error message with one that
shows the specific ungrouped variable being complained of. Perhaps thiswill reduce user confusion...
1 parentd65a27f commitf7f41c7

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
* AUTHORDATEMAJOR 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
typedefstruct {
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+
*notcheck 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 listand 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_contextcontext;
448451

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

454457
staticbool
@@ -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-
boolcontained_in_group_clause= false;
478+
Var*var;
479+
boolcontained_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
returnexpression_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+
typedefstruct {
27+
ParseState*pstate;
28+
List*groupClauses;
29+
}check_ungrouped_columns_context;
30+
2531
staticboolcontain_agg_clause(Node*clause);
2632
staticboolcontain_agg_clause_walker(Node*node,void*context);
27-
staticboolexprIsAggOrGroupCol(Node*expr,List*groupClauses);
28-
staticboolexprIsAggOrGroupCol_walker(Node*node,List*groupClauses);
33+
staticvoidcheck_ungrouped_columns(Node*node,ParseState*pstate,
34+
List*groupClauses);
35+
staticboolcheck_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-
staticbool
72-
exprIsAggOrGroupCol(Node*expr,List*groupClauses)
81+
staticvoid
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_contextcontext;
86+
87+
context.pstate=pstate;
88+
context.groupClauses=groupClauses;
89+
check_ungrouped_columns_walker(node,&context);
7990
}
8091

8192
staticbool
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-
returnexpression_tree_walker(node,exprIsAggOrGroupCol_walker,
114-
(void*)groupClauses);
143+
returnexpression_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 togenerate the generic "illegal use of
139-
*aggregatesin target list" message, which is outright misleading if
140-
*the problemis in WHERE.)
168+
* otherwise we are likely tocomplain about some innocent variable
169+
* inthetarget 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 byexprIsAggOrGroupCol() (this avoids repeated scans of the
150-
* targetlist within the recursiveroutines...)
179+
* for use bycheck_ungrouped_columns() (this avoids repeated scans of the
180+
* targetlist within the recursiveroutine...)
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
externList*pull_constant_clauses(List*quals,List**constantQual);
4141
externList*pull_agg_clause(Node*clause);
42-
externboolcheck_subplans_for_ungrouped_vars(Node*clause,
43-
List*groupClause,
42+
externvoidcheck_subplans_for_ungrouped_vars(Node*clause,
43+
Query*query,
4444
List*targetList);
4545

4646
externvoidclause_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 aggregatesornon-group columnintarget list
35+
ERROR:Attribute test_missing_target.b must be GROUPedorusedinan 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 aggregatesornon-group columnintarget list
197+
ERROR:Attribute test_missing_target.b must be GROUPedorusedinan 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)
[8]ページ先頭
©2009-2025 Movatter.jp