From eaccfded98a9c677d3a2e849c1747ec90e8596a6 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 10 Aug 2012 11:35:33 -0400 Subject: [PATCH] Centralize the logic for detecting misplaced aggregates, window funcs, etc. Formerly we relied on checking after-the-fact to see if an expression contained aggregates, window functions, or sub-selects when it shouldn't. This is grotty, easily forgotten (indeed, we had forgotten to teach DefineIndex about rejecting window functions), and none too efficient since it requires extra traversals of the parse tree. To improve matters, define an enum type that classifies all SQL sub-expressions, store it in ParseState to show what kind of expression we are currently parsing, and make transformAggregateCall, transformWindowFuncCall, and transformSubLink check the expression type and throw error if the type indicates the construct is disallowed. This allows removal of a large number of ad-hoc checks scattered around the code base. The enum type is sufficiently fine-grained that we can still produce error messages of at least the same specificity as before. Bringing these error checks together revealed that we'd been none too consistent about phrasing of the error messages, so standardize the wording a bit. Also, rewrite checking of aggregate arguments so that it requires only one traversal of the arguments, rather than up to three as before. In passing, clean up some more comments left over from add_missing_from support, and annotate some tests that I think are dead code now that that's gone. (I didn't risk actually removing said dead code, though.) --- src/backend/catalog/heap.c | 45 +- src/backend/commands/functioncmds.c | 24 +- src/backend/commands/indexcmds.c | 26 +- src/backend/commands/prepare.c | 16 +- src/backend/commands/tablecmds.c | 17 +- src/backend/commands/trigger.c | 17 +- src/backend/commands/typecmds.c | 33 +- src/backend/optimizer/util/clauses.c | 2 +- src/backend/optimizer/util/var.c | 161 ------- src/backend/parser/analyze.c | 138 ++---- src/backend/parser/parse_agg.c | 546 ++++++++++++++++------- src/backend/parser/parse_clause.c | 204 +++++---- src/backend/parser/parse_expr.c | 274 ++++++++++-- src/backend/parser/parse_node.c | 4 +- src/backend/parser/parse_target.c | 77 ++-- src/backend/parser/parse_utilcmd.c | 29 +- src/backend/rewrite/rewriteManip.c | 17 +- src/include/optimizer/var.h | 1 - src/include/parser/parse_agg.h | 1 - src/include/parser/parse_clause.h | 9 +- src/include/parser/parse_expr.h | 4 +- src/include/parser/parse_node.h | 49 ++ src/include/parser/parse_target.h | 9 +- src/include/rewrite/rewriteManip.h | 3 +- src/test/regress/expected/aggregates.out | 11 +- src/test/regress/expected/join.out | 2 +- src/test/regress/expected/window.out | 14 +- src/test/regress/expected/with.out | 4 +- src/test/regress/sql/aggregates.sql | 4 + 29 files changed, 956 insertions(+), 785 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index c91df90038..a08830121f 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2415,10 +2415,11 @@ cookDefault(ParseState *pstate, /* * Transform raw parsetree to executable expression. */ - expr = transformExpr(pstate, raw_default); + expr = transformExpr(pstate, raw_default, EXPR_KIND_COLUMN_DEFAULT); /* - * Make sure default expr does not refer to any vars. + * Make sure default expr does not refer to any vars (we need this check + * since the pstate includes the target table). */ if (contain_var_clause(expr)) ereport(ERROR, @@ -2426,6 +2427,9 @@ cookDefault(ParseState *pstate, errmsg("cannot use column references in default expression"))); /* + * transformExpr() should have already rejected subqueries, aggregates, + * and window functions, based on the EXPR_KIND_ for a default expression. + * * It can't return a set either. */ if (expression_returns_set(expr)) @@ -2433,22 +2437,6 @@ cookDefault(ParseState *pstate, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg("default expression must not return a set"))); - /* - * No subplans or aggregates, either... - */ - if (pstate->p_hasSubLinks) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use subquery in default expression"))); - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in default expression"))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in default expression"))); - /* * Coerce the expression to the correct type and typmod, if given. This * should match the parser's processing of non-defaulted expressions --- @@ -2499,7 +2487,7 @@ cookConstraint(ParseState *pstate, /* * Transform raw parsetree to executable expression. */ - expr = transformExpr(pstate, raw_constraint); + expr = transformExpr(pstate, raw_constraint, EXPR_KIND_CHECK_CONSTRAINT); /* * Make sure it yields a boolean result. @@ -2512,7 +2500,8 @@ cookConstraint(ParseState *pstate, assign_expr_collations(pstate, expr); /* - * Make sure no outside relations are referred to. + * Make sure no outside relations are referred to (this is probably dead + * code now that add_missing_from is history). */ if (list_length(pstate->p_rtable) != 1) ereport(ERROR, @@ -2520,22 +2509,6 @@ cookConstraint(ParseState *pstate, errmsg("only table \"%s\" can be referenced in check constraint", relname))); - /* - * No subplans or aggregates, either... - */ - if (pstate->p_hasSubLinks) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use subquery in check constraint"))); - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in check constraint"))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in check constraint"))); - return expr; } diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index 9ba6dd8fcf..dcadb3dc84 100644 --- a/src/backend/commands/functioncmds.c +++ b/src/backend/commands/functioncmds.c @@ -344,12 +344,14 @@ examine_parameter_list(List *parameters, Oid languageOid, (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION), errmsg("only input parameters can have default values"))); - def = transformExpr(pstate, fp->defexpr); + def = transformExpr(pstate, fp->defexpr, + EXPR_KIND_FUNCTION_DEFAULT); def = coerce_to_specific_type(pstate, def, toid, "DEFAULT"); assign_expr_collations(pstate, def); /* - * Make sure no variables are referred to. + * Make sure no variables are referred to (this is probably dead + * code now that add_missing_from is history). */ if (list_length(pstate->p_rtable) != 0 || contain_var_clause(def)) @@ -358,28 +360,18 @@ examine_parameter_list(List *parameters, Oid languageOid, errmsg("cannot use table references in parameter default value"))); /* + * transformExpr() should have already rejected subqueries, + * aggregates, and window functions, based on the EXPR_KIND_ for a + * default expression. + * * It can't return a set either --- but coerce_to_specific_type * already checked that for us. * - * No subplans or aggregates, either... - * * Note: the point of these restrictions is to ensure that an * expression that, on its face, hasn't got subplans, aggregates, * etc cannot suddenly have them after function default arguments * are inserted. */ - if (pstate->p_hasSubLinks) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use subquery in parameter default value"))); - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in parameter default value"))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in parameter default value"))); *parameterDefaults = lappend(*parameterDefaults, def); have_defaults = true; diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index a0840d1bf0..f677268609 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -941,17 +941,9 @@ static void CheckPredicate(Expr *predicate) { /* - * We don't currently support generation of an actual query plan for a - * predicate, only simple scalar expressions; hence these restrictions. + * transformExpr() should have already rejected subqueries, aggregates, + * and window functions, based on the EXPR_KIND_ for a predicate. */ - if (contain_subplans((Node *) predicate)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use subquery in index predicate"))); - if (contain_agg_clause((Node *) predicate)) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate in index predicate"))); /* * A predicate using mutable functions is probably wrong, for the same @@ -1072,18 +1064,10 @@ ComputeIndexAttrs(IndexInfo *indexInfo, expr); /* - * We don't currently support generation of an actual query - * plan for an index expression, only simple scalar - * expressions; hence these restrictions. + * transformExpr() should have already rejected subqueries, + * aggregates, and window functions, based on the EXPR_KIND_ + * for an index expression. */ - if (contain_subplans(expr)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use subquery in index expression"))); - if (contain_agg_clause(expr)) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in index expression"))); /* * A expression using mutable functions is probably wrong, diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c index 2d87b1c690..9f993de6f1 100644 --- a/src/backend/commands/prepare.c +++ b/src/backend/commands/prepare.c @@ -354,21 +354,7 @@ EvaluateParams(PreparedStatement *pstmt, List *params, Oid expected_type_id = param_types[i]; Oid given_type_id; - expr = transformExpr(pstate, expr); - - /* Cannot contain subselects or aggregates */ - if (pstate->p_hasSubLinks) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use subquery in EXECUTE parameter"))); - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in EXECUTE parameter"))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in EXECUTE parameter"))); + expr = transformExpr(pstate, expr, EXPR_KIND_EXECUTE_PARAMETER); given_type_id = exprType(expr); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 8e611e8f5c..a69544853f 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7178,27 +7178,14 @@ ATPrepAlterColumnType(List **wqueue, true); addRTEtoQuery(pstate, rte, false, true, true); - transform = transformExpr(pstate, transform); + transform = transformExpr(pstate, transform, + EXPR_KIND_ALTER_COL_TRANSFORM); /* It can't return a set */ if (expression_returns_set(transform)) ereport(ERROR, (errcode(ERRCODE_DATATYPE_MISMATCH), errmsg("transform expression must not return a set"))); - - /* No subplans or aggregates, either... */ - if (pstate->p_hasSubLinks) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use subquery in transform expression"))); - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in transform expression"))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in transform expression"))); } else { diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 5bea202240..e7576fc9ea 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -286,26 +286,11 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, /* Transform expression. Copy to be sure we don't modify original */ whenClause = transformWhereClause(pstate, copyObject(stmt->whenClause), + EXPR_KIND_TRIGGER_WHEN, "WHEN"); /* we have to fix its collations too */ assign_expr_collations(pstate, whenClause); - /* - * No subplans or aggregates, please - */ - if (pstate->p_hasSubLinks) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use subquery in trigger WHEN condition"))); - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in trigger WHEN condition"))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in trigger WHEN condition"))); - /* * Check for disallowed references to OLD/NEW. * diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index 353043d581..7fc3ad7e73 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -2871,7 +2871,7 @@ domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid, pstate->p_value_substitute = (Node *) domVal; - expr = transformExpr(pstate, constr->raw_expr); + expr = transformExpr(pstate, constr->raw_expr, EXPR_KIND_DOMAIN_CHECK); /* * Make sure it yields a boolean result. @@ -2884,38 +2884,15 @@ domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid, assign_expr_collations(pstate, expr); /* - * Make sure no outside relations are referred to. + * Domains don't allow variables (this is probably dead code now that + * add_missing_from is history, but let's be sure). */ - if (list_length(pstate->p_rtable) != 0) + if (list_length(pstate->p_rtable) != 0 || + contain_var_clause(expr)) ereport(ERROR, (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), errmsg("cannot use table references in domain check constraint"))); - /* - * Domains don't allow var clauses (this should be redundant with the - * above check, but make it anyway) - */ - if (contain_var_clause(expr)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), - errmsg("cannot use table references in domain check constraint"))); - - /* - * No subplans or aggregates, either... - */ - if (pstate->p_hasSubLinks) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use subquery in check constraint"))); - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in check constraint"))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in check constraint"))); - /* * Convert to string form for storage. */ diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index c2d36ca38d..c339f13228 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -596,7 +596,7 @@ count_agg_clauses_walker(Node *node, count_agg_clauses_context *context) bool contain_window_function(Node *clause) { - return checkExprHasWindowFuncs(clause); + return contain_windowfuncs(clause); } /* diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index 81332ff1cd..1d88a77820 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -52,12 +52,6 @@ typedef struct int sublevels_up; } locate_var_of_level_context; -typedef struct -{ - int min_varlevel; - int sublevels_up; -} find_minimum_var_level_context; - typedef struct { List *varlist; @@ -81,8 +75,6 @@ static bool contain_var_clause_walker(Node *node, void *context); static bool contain_vars_of_level_walker(Node *node, int *sublevels_up); static bool locate_var_of_level_walker(Node *node, locate_var_of_level_context *context); -static bool find_minimum_var_level_walker(Node *node, - find_minimum_var_level_context *context); static bool pull_var_clause_walker(Node *node, pull_var_clause_context *context); static Node *flatten_join_alias_vars_mutator(Node *node, @@ -488,159 +480,6 @@ locate_var_of_level_walker(Node *node, } -/* - * find_minimum_var_level - * Recursively scan a clause to find the lowest variable level it - * contains --- for example, zero is returned if there are any local - * variables, one if there are no local variables but there are - * one-level-up outer references, etc. Subqueries are scanned to see - * if they possess relevant outer references. (But any local variables - * within subqueries are not relevant.) - * - * -1 is returned if the clause has no variables at all. - * - * Will recurse into sublinks. Also, may be invoked directly on a Query. - */ -int -find_minimum_var_level(Node *node) -{ - find_minimum_var_level_context context; - - context.min_varlevel = -1; /* signifies nothing found yet */ - context.sublevels_up = 0; - - (void) query_or_expression_tree_walker(node, - find_minimum_var_level_walker, - (void *) &context, - 0); - - return context.min_varlevel; -} - -static bool -find_minimum_var_level_walker(Node *node, - find_minimum_var_level_context *context) -{ - if (node == NULL) - return false; - if (IsA(node, Var)) - { - int varlevelsup = ((Var *) node)->varlevelsup; - - /* convert levelsup to frame of reference of original query */ - varlevelsup -= context->sublevels_up; - /* ignore local vars of subqueries */ - if (varlevelsup >= 0) - { - if (context->min_varlevel < 0 || - context->min_varlevel > varlevelsup) - { - context->min_varlevel = varlevelsup; - - /* - * As soon as we find a local variable, we can abort the tree - * traversal, since min_varlevel is then certainly 0. - */ - if (varlevelsup == 0) - return true; - } - } - } - if (IsA(node, CurrentOfExpr)) - { - int varlevelsup = 0; - - /* convert levelsup to frame of reference of original query */ - varlevelsup -= context->sublevels_up; - /* ignore local vars of subqueries */ - if (varlevelsup >= 0) - { - if (context->min_varlevel < 0 || - context->min_varlevel > varlevelsup) - { - context->min_varlevel = varlevelsup; - - /* - * As soon as we find a local variable, we can abort the tree - * traversal, since min_varlevel is then certainly 0. - */ - if (varlevelsup == 0) - return true; - } - } - } - - /* - * An Aggref must be treated like a Var of its level. Normally we'd get - * the same result from looking at the Vars in the aggregate's argument, - * but this fails in the case of a Var-less aggregate call (COUNT(*)). - */ - if (IsA(node, Aggref)) - { - int agglevelsup = ((Aggref *) node)->agglevelsup; - - /* convert levelsup to frame of reference of original query */ - agglevelsup -= context->sublevels_up; - /* ignore local aggs of subqueries */ - if (agglevelsup >= 0) - { - if (context->min_varlevel < 0 || - context->min_varlevel > agglevelsup) - { - context->min_varlevel = agglevelsup; - - /* - * As soon as we find a local aggregate, we can abort the tree - * traversal, since min_varlevel is then certainly 0. - */ - if (agglevelsup == 0) - return true; - } - } - } - /* Likewise, make sure PlaceHolderVar is treated correctly */ - if (IsA(node, PlaceHolderVar)) - { - int phlevelsup = ((PlaceHolderVar *) node)->phlevelsup; - - /* convert levelsup to frame of reference of original query */ - phlevelsup -= context->sublevels_up; - /* ignore local vars of subqueries */ - if (phlevelsup >= 0) - { - if (context->min_varlevel < 0 || - context->min_varlevel > phlevelsup) - { - context->min_varlevel = phlevelsup; - - /* - * As soon as we find a local variable, we can abort the tree - * traversal, since min_varlevel is then certainly 0. - */ - if (phlevelsup == 0) - return true; - } - } - } - if (IsA(node, Query)) - { - /* Recurse into subselects */ - bool result; - - context->sublevels_up++; - result = query_tree_walker((Query *) node, - find_minimum_var_level_walker, - (void *) context, - 0); - context->sublevels_up--; - return result; - } - return expression_tree_walker(node, - find_minimum_var_level_walker, - (void *) context); -} - - /* * pull_var_clause * Recursively pulls all Var nodes from an expression clause. diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index 93ef724fff..6c3d89a14f 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -368,7 +368,8 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt) */ transformFromClause(pstate, stmt->usingClause); - qual = transformWhereClause(pstate, stmt->whereClause, "WHERE"); + qual = transformWhereClause(pstate, stmt->whereClause, + EXPR_KIND_WHERE, "WHERE"); qry->returningList = transformReturningList(pstate, stmt->returningList); @@ -378,8 +379,6 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt) qry->hasSubLinks = pstate->p_hasSubLinks; qry->hasWindowFuncs = pstate->p_hasWindowFuncs; - if (pstate->p_hasWindowFuncs) - parseCheckWindowFuncs(pstate, qry); qry->hasAggs = pstate->p_hasAggs; if (pstate->p_hasAggs) parseCheckAggregates(pstate, qry); @@ -597,7 +596,7 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt) List *sublist = (List *) lfirst(lc); /* Do basic expression transformation (same as a ROW() expr) */ - sublist = transformExpressionList(pstate, sublist); + sublist = transformExpressionList(pstate, sublist, EXPR_KIND_VALUES); /* * All the sublists must be the same length, *after* @@ -680,16 +679,11 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt) } else { - /*---------- - * Process INSERT ... VALUES with a single VALUES sublist. - * We treat this separately for efficiency and for historical - * compatibility --- specifically, allowing table references, - * such as - * INSERT INTO foo VALUES(bar.*) - * - * The sublist is just computed directly as the Query's targetlist, - * with no VALUES RTE. So it works just like SELECT without FROM. - *---------- + /* + * Process INSERT ... VALUES with a single VALUES sublist. We treat + * this case separately for efficiency. The sublist is just computed + * directly as the Query's targetlist, with no VALUES RTE. So it + * works just like a SELECT without any FROM. */ List *valuesLists = selectStmt->valuesLists; @@ -698,7 +692,8 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt) /* Do basic expression transformation (same as a ROW() expr) */ exprList = transformExpressionList(pstate, - (List *) linitial(valuesLists)); + (List *) linitial(valuesLists), + EXPR_KIND_VALUES); /* Prepare row for assignment to target table */ exprList = transformInsertRow(pstate, exprList, @@ -758,19 +753,6 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt) qry->jointree = makeFromExpr(pstate->p_joinlist, NULL); qry->hasSubLinks = pstate->p_hasSubLinks; - /* aggregates not allowed (but subselects are okay) */ - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in VALUES"), - parser_errposition(pstate, - locate_agg_of_level((Node *) qry, 0)))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in VALUES"), - parser_errposition(pstate, - locate_windowfunc((Node *) qry)))); assign_query_collations(pstate, qry); @@ -845,6 +827,7 @@ transformInsertRow(ParseState *pstate, List *exprlist, Assert(IsA(col, ResTarget)); expr = transformAssignedExpr(pstate, expr, + EXPR_KIND_INSERT_TARGET, col->name, lfirst_int(attnos), col->indirection, @@ -945,19 +928,19 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt) transformFromClause(pstate, stmt->fromClause); /* transform targetlist */ - qry->targetList = transformTargetList(pstate, stmt->targetList); + qry->targetList = transformTargetList(pstate, stmt->targetList, + EXPR_KIND_SELECT_TARGET); /* mark column origins */ markTargetListOrigins(pstate, qry->targetList); /* transform WHERE */ - qual = transformWhereClause(pstate, stmt->whereClause, "WHERE"); + qual = transformWhereClause(pstate, stmt->whereClause, + EXPR_KIND_WHERE, "WHERE"); - /* - * Initial processing of HAVING clause is just like WHERE clause. - */ + /* initial processing of HAVING clause is much like WHERE clause */ qry->havingQual = transformWhereClause(pstate, stmt->havingClause, - "HAVING"); + EXPR_KIND_HAVING, "HAVING"); /* * Transform sorting/grouping stuff. Do ORDER BY first because both @@ -968,6 +951,7 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt) qry->sortClause = transformSortClause(pstate, stmt->sortClause, &qry->targetList, + EXPR_KIND_ORDER_BY, true /* fix unknowns */ , false /* allow SQL92 rules */ ); @@ -975,6 +959,7 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt) stmt->groupClause, &qry->targetList, qry->sortClause, + EXPR_KIND_GROUP_BY, false /* allow SQL92 rules */ ); if (stmt->distinctClause == NIL) @@ -1003,9 +988,9 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt) /* transform LIMIT */ qry->limitOffset = transformLimitClause(pstate, stmt->limitOffset, - "OFFSET"); + EXPR_KIND_OFFSET, "OFFSET"); qry->limitCount = transformLimitClause(pstate, stmt->limitCount, - "LIMIT"); + EXPR_KIND_LIMIT, "LIMIT"); /* transform window clauses after we have seen all window functions */ qry->windowClause = transformWindowDefinitions(pstate, @@ -1017,8 +1002,6 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt) qry->hasSubLinks = pstate->p_hasSubLinks; qry->hasWindowFuncs = pstate->p_hasWindowFuncs; - if (pstate->p_hasWindowFuncs) - parseCheckWindowFuncs(pstate, qry); qry->hasAggs = pstate->p_hasAggs; if (pstate->p_hasAggs || qry->groupClause || qry->havingQual) parseCheckAggregates(pstate, qry); @@ -1090,7 +1073,7 @@ transformValuesClause(ParseState *pstate, SelectStmt *stmt) List *sublist = (List *) lfirst(lc); /* Do basic expression transformation (same as a ROW() expr) */ - sublist = transformExpressionList(pstate, sublist); + sublist = transformExpressionList(pstate, sublist, EXPR_KIND_VALUES); /* * All the sublists must be the same length, *after* transformation @@ -1217,13 +1200,14 @@ transformValuesClause(ParseState *pstate, SelectStmt *stmt) qry->sortClause = transformSortClause(pstate, stmt->sortClause, &qry->targetList, + EXPR_KIND_ORDER_BY, true /* fix unknowns */ , false /* allow SQL92 rules */ ); qry->limitOffset = transformLimitClause(pstate, stmt->limitOffset, - "OFFSET"); + EXPR_KIND_OFFSET, "OFFSET"); qry->limitCount = transformLimitClause(pstate, stmt->limitCount, - "LIMIT"); + EXPR_KIND_LIMIT, "LIMIT"); if (stmt->lockingClause) ereport(ERROR, @@ -1249,19 +1233,6 @@ transformValuesClause(ParseState *pstate, SelectStmt *stmt) qry->jointree = makeFromExpr(pstate->p_joinlist, NULL); qry->hasSubLinks = pstate->p_hasSubLinks; - /* aggregates not allowed (but subselects are okay) */ - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in VALUES"), - parser_errposition(pstate, - locate_agg_of_level((Node *) exprsLists, 0)))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in VALUES"), - parser_errposition(pstate, - locate_windowfunc((Node *) exprsLists)))); assign_query_collations(pstate, qry); @@ -1460,6 +1431,7 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt) qry->sortClause = transformSortClause(pstate, sortClause, &qry->targetList, + EXPR_KIND_ORDER_BY, false /* no unknowns expected */ , false /* allow SQL92 rules */ ); @@ -1477,17 +1449,15 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt) exprLocation(list_nth(qry->targetList, tllen))))); qry->limitOffset = transformLimitClause(pstate, limitOffset, - "OFFSET"); + EXPR_KIND_OFFSET, "OFFSET"); qry->limitCount = transformLimitClause(pstate, limitCount, - "LIMIT"); + EXPR_KIND_LIMIT, "LIMIT"); qry->rtable = pstate->p_rtable; qry->jointree = makeFromExpr(pstate->p_joinlist, NULL); qry->hasSubLinks = pstate->p_hasSubLinks; qry->hasWindowFuncs = pstate->p_hasWindowFuncs; - if (pstate->p_hasWindowFuncs) - parseCheckWindowFuncs(pstate, qry); qry->hasAggs = pstate->p_hasAggs; if (pstate->p_hasAggs || qry->groupClause || qry->havingQual) parseCheckAggregates(pstate, qry); @@ -1937,9 +1907,11 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt) */ transformFromClause(pstate, stmt->fromClause); - qry->targetList = transformTargetList(pstate, stmt->targetList); + qry->targetList = transformTargetList(pstate, stmt->targetList, + EXPR_KIND_UPDATE_SOURCE); - qual = transformWhereClause(pstate, stmt->whereClause, "WHERE"); + qual = transformWhereClause(pstate, stmt->whereClause, + EXPR_KIND_WHERE, "WHERE"); qry->returningList = transformReturningList(pstate, stmt->returningList); @@ -1948,24 +1920,6 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt) qry->hasSubLinks = pstate->p_hasSubLinks; - /* - * Top-level aggregates are simply disallowed in UPDATE, per spec. (From - * an implementation point of view, this is forced because the implicit - * ctid reference would otherwise be an ungrouped variable.) - */ - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in UPDATE"), - parser_errposition(pstate, - locate_agg_of_level((Node *) qry, 0)))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in UPDATE"), - parser_errposition(pstate, - locate_windowfunc((Node *) qry)))); - /* * Now we are done with SELECT-like processing, and can get on with * transforming the target list to match the UPDATE target columns. @@ -2040,8 +1994,6 @@ transformReturningList(ParseState *pstate, List *returningList) { List *rlist; int save_next_resno; - bool save_hasAggs; - bool save_hasWindowFuncs; if (returningList == NIL) return NIL; /* nothing to do */ @@ -2054,38 +2006,14 @@ transformReturningList(ParseState *pstate, List *returningList) save_next_resno = pstate->p_next_resno; pstate->p_next_resno = 1; - /* save other state so that we can detect disallowed stuff */ - save_hasAggs = pstate->p_hasAggs; - pstate->p_hasAggs = false; - save_hasWindowFuncs = pstate->p_hasWindowFuncs; - pstate->p_hasWindowFuncs = false; - /* transform RETURNING identically to a SELECT targetlist */ - rlist = transformTargetList(pstate, returningList); - - /* check for disallowed stuff */ - - /* aggregates not allowed (but subselects are okay) */ - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in RETURNING"), - parser_errposition(pstate, - locate_agg_of_level((Node *) rlist, 0)))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in RETURNING"), - parser_errposition(pstate, - locate_windowfunc((Node *) rlist)))); + rlist = transformTargetList(pstate, returningList, EXPR_KIND_RETURNING); /* mark column origins */ markTargetListOrigins(pstate, rlist); /* restore state */ pstate->p_next_resno = save_next_resno; - pstate->p_hasAggs = save_hasAggs; - pstate->p_hasWindowFuncs = save_hasWindowFuncs; return rlist; } diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index 5854f81005..d1d835b800 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -20,11 +20,20 @@ #include "optimizer/tlist.h" #include "parser/parse_agg.h" #include "parser/parse_clause.h" +#include "parser/parse_expr.h" #include "parser/parsetree.h" #include "rewrite/rewriteManip.h" #include "utils/builtins.h" +typedef struct +{ + ParseState *pstate; + int min_varlevel; + int min_agglevel; + int sublevels_up; +} check_agg_arguments_context; + typedef struct { ParseState *pstate; @@ -35,6 +44,9 @@ typedef struct int sublevels_up; } check_ungrouped_columns_context; +static int check_agg_arguments(ParseState *pstate, List *args); +static bool check_agg_arguments_walker(Node *node, + check_agg_arguments_context *context); static void check_ungrouped_columns(Node *node, ParseState *pstate, Query *qry, List *groupClauses, bool have_non_var_grouping, List **func_grouped_rels); @@ -72,6 +84,8 @@ transformAggregateCall(ParseState *pstate, Aggref *agg, int save_next_resno; int min_varlevel; ListCell *lc; + const char *err; + bool errkind; /* * Transform the plain list of Exprs into a targetlist. We don't bother @@ -102,6 +116,7 @@ transformAggregateCall(ParseState *pstate, Aggref *agg, torder = transformSortClause(pstate, aggorder, &tlist, + EXPR_KIND_ORDER_BY, true /* fix unknowns */ , true /* force SQL99 rules */ ); @@ -142,55 +157,261 @@ transformAggregateCall(ParseState *pstate, Aggref *agg, pstate->p_next_resno = save_next_resno; /* - * The aggregate's level is the same as the level of the lowest-level - * variable or aggregate in its arguments; or if it contains no variables - * at all, we presume it to be local. + * Check the arguments to compute the aggregate's level and detect + * improper nesting. */ - min_varlevel = find_minimum_var_level((Node *) agg->args); - - /* - * An aggregate can't directly contain another aggregate call of the same - * level (though outer aggs are okay). We can skip this check if we - * didn't find any local vars or aggs. - */ - if (min_varlevel == 0) - { - if (pstate->p_hasAggs && - checkExprHasAggs((Node *) agg->args)) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("aggregate function calls cannot be nested"), - parser_errposition(pstate, - locate_agg_of_level((Node *) agg->args, 0)))); - } - - /* It can't contain window functions either */ - if (pstate->p_hasWindowFuncs && - checkExprHasWindowFuncs((Node *) agg->args)) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("aggregate function calls cannot contain window function calls"), - parser_errposition(pstate, - locate_windowfunc((Node *) agg->args)))); - - if (min_varlevel < 0) - min_varlevel = 0; + min_varlevel = check_agg_arguments(pstate, agg->args); agg->agglevelsup = min_varlevel; - /* Mark the correct pstate as having aggregates */ + /* Mark the correct pstate level as having aggregates */ while (min_varlevel-- > 0) pstate = pstate->parentParseState; pstate->p_hasAggs = true; /* - * Complain if we are inside a LATERAL subquery of the aggregation query. - * We must be in its FROM clause, so the aggregate is misplaced. + * Check to see if the aggregate function is in an invalid place within + * its aggregation query. + * + * For brevity we support two schemes for reporting an error here: set + * "err" to a custom message, or set "errkind" true if the error context + * is sufficiently identified by what ParseExprKindName will return, *and* + * what it will return is just a SQL keyword. (Otherwise, use a custom + * message to avoid creating translation problems.) */ - if (pstate->p_lateral_active) + err = NULL; + errkind = false; + switch (pstate->p_expr_kind) + { + case EXPR_KIND_NONE: + Assert(false); /* can't happen */ + break; + case EXPR_KIND_OTHER: + /* Accept aggregate here; caller must throw error if wanted */ + break; + case EXPR_KIND_JOIN_ON: + case EXPR_KIND_JOIN_USING: + err = _("aggregate functions are not allowed in JOIN conditions"); + break; + case EXPR_KIND_FROM_SUBSELECT: + /* Should only be possible in a LATERAL subquery */ + Assert(pstate->p_lateral_active); + /* Aggregate scope rules make it worth being explicit here */ + err = _("aggregate functions are not allowed in FROM clause of their own query level"); + break; + case EXPR_KIND_FROM_FUNCTION: + err = _("aggregate functions are not allowed in functions in FROM"); + break; + case EXPR_KIND_WHERE: + errkind = true; + break; + case EXPR_KIND_HAVING: + /* okay */ + break; + case EXPR_KIND_WINDOW_PARTITION: + /* okay */ + break; + case EXPR_KIND_WINDOW_ORDER: + /* okay */ + break; + case EXPR_KIND_WINDOW_FRAME_RANGE: + err = _("aggregate functions are not allowed in window RANGE"); + break; + case EXPR_KIND_WINDOW_FRAME_ROWS: + err = _("aggregate functions are not allowed in window ROWS"); + break; + case EXPR_KIND_SELECT_TARGET: + /* okay */ + break; + case EXPR_KIND_INSERT_TARGET: + case EXPR_KIND_UPDATE_SOURCE: + case EXPR_KIND_UPDATE_TARGET: + errkind = true; + break; + case EXPR_KIND_GROUP_BY: + errkind = true; + break; + case EXPR_KIND_ORDER_BY: + /* okay */ + break; + case EXPR_KIND_DISTINCT_ON: + /* okay */ + break; + case EXPR_KIND_LIMIT: + case EXPR_KIND_OFFSET: + errkind = true; + break; + case EXPR_KIND_RETURNING: + errkind = true; + break; + case EXPR_KIND_VALUES: + errkind = true; + break; + case EXPR_KIND_CHECK_CONSTRAINT: + case EXPR_KIND_DOMAIN_CHECK: + err = _("aggregate functions are not allowed in CHECK constraints"); + break; + case EXPR_KIND_COLUMN_DEFAULT: + case EXPR_KIND_FUNCTION_DEFAULT: + err = _("aggregate functions are not allowed in DEFAULT expressions"); + break; + case EXPR_KIND_INDEX_EXPRESSION: + err = _("aggregate functions are not allowed in index expressions"); + break; + case EXPR_KIND_INDEX_PREDICATE: + err = _("aggregate functions are not allowed in index predicates"); + break; + case EXPR_KIND_ALTER_COL_TRANSFORM: + err = _("aggregate functions are not allowed in transform expressions"); + break; + case EXPR_KIND_EXECUTE_PARAMETER: + err = _("aggregate functions are not allowed in EXECUTE parameters"); + break; + case EXPR_KIND_TRIGGER_WHEN: + err = _("aggregate functions are not allowed in trigger WHEN conditions"); + break; + + /* + * There is intentionally no default: case here, so that the + * compiler will warn if we add a new ParseExprKind without + * extending this switch. If we do see an unrecognized value at + * runtime, the behavior will be the same as for EXPR_KIND_OTHER, + * which is sane anyway. + */ + } + if (err) ereport(ERROR, (errcode(ERRCODE_GROUPING_ERROR), - errmsg("aggregates not allowed in FROM clause"), + errmsg_internal("%s", err), parser_errposition(pstate, agg->location))); + if (errkind) + ereport(ERROR, + (errcode(ERRCODE_GROUPING_ERROR), + /* translator: %s is name of a SQL construct, eg GROUP BY */ + errmsg("aggregate functions are not allowed in %s", + ParseExprKindName(pstate->p_expr_kind)), + parser_errposition(pstate, agg->location))); +} + +/* + * check_agg_arguments + * Scan the arguments of an aggregate function to determine the + * aggregate's semantic level (zero is the current select's level, + * one is its parent, etc). + * + * The aggregate's level is the same as the level of the lowest-level variable + * or aggregate in its arguments; or if it contains no variables at all, we + * presume it to be local. + * + * We also take this opportunity to detect any aggregates or window functions + * nested within the arguments. We can throw error immediately if we find + * a window function. Aggregates are a bit trickier because it's only an + * error if the inner aggregate is of the same semantic level as the outer, + * which we can't know until we finish scanning the arguments. + */ +static int +check_agg_arguments(ParseState *pstate, List *args) +{ + int agglevel; + check_agg_arguments_context context; + + context.pstate = pstate; + context.min_varlevel = -1; /* signifies nothing found yet */ + context.min_agglevel = -1; + context.sublevels_up = 0; + + (void) expression_tree_walker((Node *) args, + check_agg_arguments_walker, + (void *) &context); + + /* + * If we found no vars nor aggs at all, it's a level-zero aggregate; + * otherwise, its level is the minimum of vars or aggs. + */ + if (context.min_varlevel < 0) + { + if (context.min_agglevel < 0) + return 0; + agglevel = context.min_agglevel; + } + else if (context.min_agglevel < 0) + agglevel = context.min_varlevel; + else + agglevel = Min(context.min_varlevel, context.min_agglevel); + + /* + * If there's a nested aggregate of the same semantic level, complain. + */ + if (agglevel == context.min_agglevel) + ereport(ERROR, + (errcode(ERRCODE_GROUPING_ERROR), + errmsg("aggregate function calls cannot be nested"), + parser_errposition(pstate, + locate_agg_of_level((Node *) args, + agglevel)))); + + return agglevel; +} + +static bool +check_agg_arguments_walker(Node *node, + check_agg_arguments_context *context) +{ + if (node == NULL) + return false; + if (IsA(node, Var)) + { + int varlevelsup = ((Var *) node)->varlevelsup; + + /* convert levelsup to frame of reference of original query */ + varlevelsup -= context->sublevels_up; + /* ignore local vars of subqueries */ + if (varlevelsup >= 0) + { + if (context->min_varlevel < 0 || + context->min_varlevel > varlevelsup) + context->min_varlevel = varlevelsup; + } + return false; + } + if (IsA(node, Aggref)) + { + int agglevelsup = ((Aggref *) node)->agglevelsup; + + /* convert levelsup to frame of reference of original query */ + agglevelsup -= context->sublevels_up; + /* ignore local aggs of subqueries */ + if (agglevelsup >= 0) + { + if (context->min_agglevel < 0 || + context->min_agglevel > agglevelsup) + context->min_agglevel = agglevelsup; + } + /* no need to examine args of the inner aggregate */ + return false; + } + /* We can throw error on sight for a window function */ + if (IsA(node, WindowFunc)) + ereport(ERROR, + (errcode(ERRCODE_GROUPING_ERROR), + errmsg("aggregate function calls cannot contain window function calls"), + parser_errposition(context->pstate, + ((WindowFunc *) node)->location))); + if (IsA(node, Query)) + { + /* Recurse into subselects */ + bool result; + + context->sublevels_up++; + result = query_tree_walker((Query *) node, + check_agg_arguments_walker, + (void *) context, + 0); + context->sublevels_up--; + return result; + } + return expression_tree_walker(node, + check_agg_arguments_walker, + (void *) context); } /* @@ -208,18 +429,136 @@ void transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc, WindowDef *windef) { + const char *err; + bool errkind; + /* * A window function call can't contain another one (but aggs are OK). XXX * is this required by spec, or just an unimplemented feature? */ if (pstate->p_hasWindowFuncs && - checkExprHasWindowFuncs((Node *) wfunc->args)) + contain_windowfuncs((Node *) wfunc->args)) ereport(ERROR, (errcode(ERRCODE_WINDOWING_ERROR), errmsg("window function calls cannot be nested"), parser_errposition(pstate, locate_windowfunc((Node *) wfunc->args)))); + /* + * Check to see if the window function is in an invalid place within the + * query. + * + * For brevity we support two schemes for reporting an error here: set + * "err" to a custom message, or set "errkind" true if the error context + * is sufficiently identified by what ParseExprKindName will return, *and* + * what it will return is just a SQL keyword. (Otherwise, use a custom + * message to avoid creating translation problems.) + */ + err = NULL; + errkind = false; + switch (pstate->p_expr_kind) + { + case EXPR_KIND_NONE: + Assert(false); /* can't happen */ + break; + case EXPR_KIND_OTHER: + /* Accept window func here; caller must throw error if wanted */ + break; + case EXPR_KIND_JOIN_ON: + case EXPR_KIND_JOIN_USING: + err = _("window functions are not allowed in JOIN conditions"); + break; + case EXPR_KIND_FROM_SUBSELECT: + /* can't get here, but just in case, throw an error */ + errkind = true; + break; + case EXPR_KIND_FROM_FUNCTION: + err = _("window functions are not allowed in functions in FROM"); + break; + case EXPR_KIND_WHERE: + errkind = true; + break; + case EXPR_KIND_HAVING: + errkind = true; + break; + case EXPR_KIND_WINDOW_PARTITION: + case EXPR_KIND_WINDOW_ORDER: + case EXPR_KIND_WINDOW_FRAME_RANGE: + case EXPR_KIND_WINDOW_FRAME_ROWS: + err = _("window functions are not allowed in window definitions"); + break; + case EXPR_KIND_SELECT_TARGET: + /* okay */ + break; + case EXPR_KIND_INSERT_TARGET: + case EXPR_KIND_UPDATE_SOURCE: + case EXPR_KIND_UPDATE_TARGET: + errkind = true; + break; + case EXPR_KIND_GROUP_BY: + errkind = true; + break; + case EXPR_KIND_ORDER_BY: + /* okay */ + break; + case EXPR_KIND_DISTINCT_ON: + /* okay */ + break; + case EXPR_KIND_LIMIT: + case EXPR_KIND_OFFSET: + errkind = true; + break; + case EXPR_KIND_RETURNING: + errkind = true; + break; + case EXPR_KIND_VALUES: + errkind = true; + break; + case EXPR_KIND_CHECK_CONSTRAINT: + case EXPR_KIND_DOMAIN_CHECK: + err = _("window functions are not allowed in CHECK constraints"); + break; + case EXPR_KIND_COLUMN_DEFAULT: + case EXPR_KIND_FUNCTION_DEFAULT: + err = _("window functions are not allowed in DEFAULT expressions"); + break; + case EXPR_KIND_INDEX_EXPRESSION: + err = _("window functions are not allowed in index expressions"); + break; + case EXPR_KIND_INDEX_PREDICATE: + err = _("window functions are not allowed in index predicates"); + break; + case EXPR_KIND_ALTER_COL_TRANSFORM: + err = _("window functions are not allowed in transform expressions"); + break; + case EXPR_KIND_EXECUTE_PARAMETER: + err = _("window functions are not allowed in EXECUTE parameters"); + break; + case EXPR_KIND_TRIGGER_WHEN: + err = _("window functions are not allowed in trigger WHEN conditions"); + break; + + /* + * There is intentionally no default: case here, so that the + * compiler will warn if we add a new ParseExprKind without + * extending this switch. If we do see an unrecognized value at + * runtime, the behavior will be the same as for EXPR_KIND_OTHER, + * which is sane anyway. + */ + } + if (err) + ereport(ERROR, + (errcode(ERRCODE_WINDOWING_ERROR), + errmsg_internal("%s", err), + parser_errposition(pstate, wfunc->location))); + if (errkind) + ereport(ERROR, + (errcode(ERRCODE_WINDOWING_ERROR), + /* translator: %s is name of a SQL construct, eg GROUP BY */ + errmsg("window functions are not allowed in %s", + ParseExprKindName(pstate->p_expr_kind)), + parser_errposition(pstate, wfunc->location))); + /* * If the OVER clause just specifies a window name, find that WINDOW * clause (which had better be present). Otherwise, try to match all the @@ -294,11 +633,14 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc, /* * parseCheckAggregates * Check for aggregates where they shouldn't be and improper grouping. + * This function should be called after the target list and qualifications + * are finalized. * - * 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. + * Misplaced aggregates are now mostly detected in transformAggregateCall, + * but it seems more robust to check for aggregates in recursive queries + * only after everything is finalized. In any case it's hard to detect + * improper grouping on-the-fly, so we have to make another pass over the + * query for that. */ void parseCheckAggregates(ParseState *pstate, Query *qry) @@ -331,31 +673,8 @@ parseCheckAggregates(ParseState *pstate, Query *qry) } /* - * Aggregates must never appear in WHERE or JOIN/ON clauses. - * - * (Note this check should appear first to deliver an appropriate error - * message; 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 (checkExprHasAggs(qry->jointree->quals)) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("aggregates not allowed in WHERE clause"), - parser_errposition(pstate, - locate_agg_of_level(qry->jointree->quals, 0)))); - if (checkExprHasAggs((Node *) qry->jointree->fromlist)) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("aggregates not allowed in JOIN conditions"), - parser_errposition(pstate, - locate_agg_of_level((Node *) qry->jointree->fromlist, 0)))); - - /* - * 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 check_ungrouped_columns(). + * Build a list of the acceptable GROUP BY expressions for use by + * check_ungrouped_columns(). */ foreach(l, qry->groupClause) { @@ -365,12 +684,6 @@ parseCheckAggregates(ParseState *pstate, Query *qry) expr = get_sortgroupclause_expr(grpcl, qry->targetList); if (expr == NULL) continue; /* probably cannot happen */ - if (checkExprHasAggs(expr)) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("aggregates not allowed in GROUP BY clause"), - parser_errposition(pstate, - locate_agg_of_level(expr, 0)))); groupClauses = lcons(expr, groupClauses); } @@ -438,96 +751,11 @@ parseCheckAggregates(ParseState *pstate, Query *qry) if (pstate->p_hasAggs && hasSelfRefRTEs) ereport(ERROR, (errcode(ERRCODE_INVALID_RECURSION), - errmsg("aggregate functions not allowed in a recursive query's recursive term"), + errmsg("aggregate functions are not allowed in a recursive query's recursive term"), parser_errposition(pstate, locate_agg_of_level((Node *) qry, 0)))); } -/* - * parseCheckWindowFuncs - * Check for window functions where they shouldn't be. - * - * We have to forbid window functions in WHERE, JOIN/ON, HAVING, GROUP BY, - * and window specifications. (Other clauses, such as RETURNING and LIMIT, - * have already been checked.) Transformation of all these clauses must - * be completed already. - */ -void -parseCheckWindowFuncs(ParseState *pstate, Query *qry) -{ - ListCell *l; - - /* This should only be called if we found window functions */ - Assert(pstate->p_hasWindowFuncs); - - if (checkExprHasWindowFuncs(qry->jointree->quals)) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("window functions not allowed in WHERE clause"), - parser_errposition(pstate, - locate_windowfunc(qry->jointree->quals)))); - if (checkExprHasWindowFuncs((Node *) qry->jointree->fromlist)) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("window functions not allowed in JOIN conditions"), - parser_errposition(pstate, - locate_windowfunc((Node *) qry->jointree->fromlist)))); - if (checkExprHasWindowFuncs(qry->havingQual)) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("window functions not allowed in HAVING clause"), - parser_errposition(pstate, - locate_windowfunc(qry->havingQual)))); - - foreach(l, qry->groupClause) - { - SortGroupClause *grpcl = (SortGroupClause *) lfirst(l); - Node *expr; - - expr = get_sortgroupclause_expr(grpcl, qry->targetList); - if (checkExprHasWindowFuncs(expr)) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("window functions not allowed in GROUP BY clause"), - parser_errposition(pstate, - locate_windowfunc(expr)))); - } - - foreach(l, qry->windowClause) - { - WindowClause *wc = (WindowClause *) lfirst(l); - ListCell *l2; - - foreach(l2, wc->partitionClause) - { - SortGroupClause *grpcl = (SortGroupClause *) lfirst(l2); - Node *expr; - - expr = get_sortgroupclause_expr(grpcl, qry->targetList); - if (checkExprHasWindowFuncs(expr)) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("window functions not allowed in window definition"), - parser_errposition(pstate, - locate_windowfunc(expr)))); - } - foreach(l2, wc->orderClause) - { - SortGroupClause *grpcl = (SortGroupClause *) lfirst(l2); - Node *expr; - - expr = get_sortgroupclause_expr(grpcl, qry->targetList); - if (checkExprHasWindowFuncs(expr)) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("window functions not allowed in window definition"), - parser_errposition(pstate, - locate_windowfunc(expr)))); - } - /* startOffset and limitOffset were checked in transformFrameOffset */ - } -} - /* * check_ungrouped_columns - * Scan the given expression tree for ungrouped variables (variables diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index d354baf42f..ee40b5547e 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -41,17 +41,6 @@ /* Convenience macro for the most common makeNamespaceItem() case */ #define makeDefaultNSItem(rte) makeNamespaceItem(rte, true, true, false, true) -/* clause types for findTargetlistEntrySQL92 */ -#define ORDER_CLAUSE 0 -#define GROUP_CLAUSE 1 -#define DISTINCT_ON_CLAUSE 2 - -static const char *const clauseText[] = { - "ORDER BY", - "GROUP BY", - "DISTINCT ON" -}; - static void extractRemainingColumns(List *common_colnames, List *src_colnames, List *src_colvars, List **res_colnames, List **res_colvars); @@ -81,9 +70,9 @@ static void setNamespaceLateralState(List *namespace, static void checkExprIsVarFree(ParseState *pstate, Node *n, const char *constructName); static TargetEntry *findTargetlistEntrySQL92(ParseState *pstate, Node *node, - List **tlist, int clause); + List **tlist, ParseExprKind exprKind); static TargetEntry *findTargetlistEntrySQL99(ParseState *pstate, Node *node, - List **tlist); + List **tlist, ParseExprKind exprKind); static int get_matching_location(int sortgroupref, List *sortgrouprefs, List *exprs); static List *addTargetToSortList(ParseState *pstate, TargetEntry *tle, @@ -371,7 +360,7 @@ transformJoinUsingClause(ParseState *pstate, * transformJoinOnClause() does. Just invoke transformExpr() to fix up * the operators, and we're done. */ - result = transformExpr(pstate, result); + result = transformExpr(pstate, result, EXPR_KIND_JOIN_USING); result = coerce_to_boolean(pstate, result, "JOIN/USING"); @@ -401,7 +390,8 @@ transformJoinOnClause(ParseState *pstate, JoinExpr *j, List *namespace) save_namespace = pstate->p_namespace; pstate->p_namespace = namespace; - result = transformWhereClause(pstate, j->quals, "JOIN/ON"); + result = transformWhereClause(pstate, j->quals, + EXPR_KIND_JOIN_ON, "JOIN/ON"); pstate->p_namespace = save_namespace; @@ -457,6 +447,14 @@ transformRangeSubselect(ParseState *pstate, RangeSubselect *r) if (r->alias == NULL) elog(ERROR, "subquery in FROM must have an alias"); + /* + * Set p_expr_kind to show this parse level is recursing to a subselect. + * We can't be nested within any expression, so don't need save-restore + * logic here. + */ + Assert(pstate->p_expr_kind == EXPR_KIND_NONE); + pstate->p_expr_kind = EXPR_KIND_FROM_SUBSELECT; + /* * If the subselect is LATERAL, make lateral_only names of this level * visible to it. (LATERAL can't nest within a single pstate level, so we @@ -471,7 +469,9 @@ transformRangeSubselect(ParseState *pstate, RangeSubselect *r) query = parse_sub_analyze(r->subquery, pstate, NULL, isLockedRefname(pstate, r->alias->aliasname)); + /* Restore state */ pstate->p_lateral_active = false; + pstate->p_expr_kind = EXPR_KIND_NONE; /* * Check that we got something reasonable. Many of these conditions are @@ -524,7 +524,7 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r) /* * Transform the raw expression. */ - funcexpr = transformExpr(pstate, r->funccallnode); + funcexpr = transformExpr(pstate, r->funccallnode, EXPR_KIND_FROM_FUNCTION); pstate->p_lateral_active = false; @@ -533,25 +533,6 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r) */ assign_expr_collations(pstate, funcexpr); - /* - * Disallow aggregate functions in the expression. (No reason to postpone - * this check until parseCheckAggregates.) - */ - if (pstate->p_hasAggs && - checkExprHasAggs(funcexpr)) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in function expression in FROM"), - parser_errposition(pstate, - locate_agg_of_level(funcexpr, 0)))); - if (pstate->p_hasWindowFuncs && - checkExprHasWindowFuncs(funcexpr)) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in function expression in FROM"), - parser_errposition(pstate, - locate_windowfunc(funcexpr)))); - /* * OK, build an RTE for the function. */ @@ -1182,14 +1163,14 @@ setNamespaceLateralState(List *namespace, bool lateral_only, bool lateral_ok) */ Node * transformWhereClause(ParseState *pstate, Node *clause, - const char *constructName) + ParseExprKind exprKind, const char *constructName) { Node *qual; if (clause == NULL) return NULL; - qual = transformExpr(pstate, clause); + qual = transformExpr(pstate, clause, exprKind); qual = coerce_to_boolean(pstate, qual, constructName); @@ -1209,18 +1190,18 @@ transformWhereClause(ParseState *pstate, Node *clause, */ Node * transformLimitClause(ParseState *pstate, Node *clause, - const char *constructName) + ParseExprKind exprKind, const char *constructName) { Node *qual; if (clause == NULL) return NULL; - qual = transformExpr(pstate, clause); + qual = transformExpr(pstate, clause, exprKind); qual = coerce_to_specific_type(pstate, qual, INT8OID, constructName); - /* LIMIT can't refer to any vars or aggregates of the current query */ + /* LIMIT can't refer to any variables of the current query */ checkExprIsVarFree(pstate, qual, constructName); return qual; @@ -1229,7 +1210,7 @@ transformLimitClause(ParseState *pstate, Node *clause, /* * checkExprIsVarFree * Check that given expr has no Vars of the current query level - * (and no aggregates or window functions, either). + * (aggregates and window functions should have been rejected already). * * This is used to check expressions that have to have a consistent value * across all rows of the query, such as a LIMIT. Arguably it should reject @@ -1251,31 +1232,57 @@ checkExprIsVarFree(ParseState *pstate, Node *n, const char *constructName) parser_errposition(pstate, locate_var_of_level(n, 0)))); } - if (pstate->p_hasAggs && - checkExprHasAggs(n)) - { - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - /* translator: %s is name of a SQL construct, eg LIMIT */ - errmsg("argument of %s must not contain aggregate functions", - constructName), - parser_errposition(pstate, - locate_agg_of_level(n, 0)))); - } - if (pstate->p_hasWindowFuncs && - checkExprHasWindowFuncs(n)) - { - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - /* translator: %s is name of a SQL construct, eg LIMIT */ - errmsg("argument of %s must not contain window functions", - constructName), - parser_errposition(pstate, - locate_windowfunc(n)))); - } } +/* + * checkTargetlistEntrySQL92 - + * Validate a targetlist entry found by findTargetlistEntrySQL92 + * + * When we select a pre-existing tlist entry as a result of syntax such + * as "GROUP BY 1", we have to make sure it is acceptable for use in the + * indicated clause type; transformExpr() will have treated it as a regular + * targetlist item. + */ +static void +checkTargetlistEntrySQL92(ParseState *pstate, TargetEntry *tle, + ParseExprKind exprKind) +{ + switch (exprKind) + { + case EXPR_KIND_GROUP_BY: + /* reject aggregates and window functions */ + if (pstate->p_hasAggs && + contain_aggs_of_level((Node *) tle->expr, 0)) + ereport(ERROR, + (errcode(ERRCODE_GROUPING_ERROR), + /* translator: %s is name of a SQL construct, eg GROUP BY */ + errmsg("aggregate functions are not allowed in %s", + ParseExprKindName(exprKind)), + parser_errposition(pstate, + locate_agg_of_level((Node *) tle->expr, 0)))); + if (pstate->p_hasWindowFuncs && + contain_windowfuncs((Node *) tle->expr)) + ereport(ERROR, + (errcode(ERRCODE_WINDOWING_ERROR), + /* translator: %s is name of a SQL construct, eg GROUP BY */ + errmsg("window functions are not allowed in %s", + ParseExprKindName(exprKind)), + parser_errposition(pstate, + locate_windowfunc((Node *) tle->expr)))); + break; + case EXPR_KIND_ORDER_BY: + /* no extra checks needed */ + break; + case EXPR_KIND_DISTINCT_ON: + /* no extra checks needed */ + break; + default: + elog(ERROR, "unexpected exprKind in checkTargetlistEntrySQL92"); + break; + } +} + /* * findTargetlistEntrySQL92 - * Returns the targetlist entry matching the given (untransformed) node. @@ -1291,11 +1298,11 @@ checkExprIsVarFree(ParseState *pstate, Node *n, const char *constructName) * * node the ORDER BY, GROUP BY, or DISTINCT ON expression to be matched * tlist the target list (passed by reference so we can append to it) - * clause identifies clause type being processed + * exprKind identifies clause type being processed */ static TargetEntry * findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, - int clause) + ParseExprKind exprKind) { ListCell *tl; @@ -1344,7 +1351,7 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, char *name = strVal(linitial(((ColumnRef *) node)->fields)); int location = ((ColumnRef *) node)->location; - if (clause == GROUP_CLAUSE) + if (exprKind == EXPR_KIND_GROUP_BY) { /* * In GROUP BY, we must prefer a match against a FROM-clause @@ -1386,7 +1393,8 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, /*------ translator: first %s is name of a SQL construct, eg ORDER BY */ errmsg("%s \"%s\" is ambiguous", - clauseText[clause], name), + ParseExprKindName(exprKind), + name), parser_errposition(pstate, location))); } else @@ -1395,7 +1403,11 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, } } if (target_result != NULL) - return target_result; /* return the first match */ + { + /* return the first match, after suitable validation */ + checkTargetlistEntrySQL92(pstate, target_result, exprKind); + return target_result; + } } } if (IsA(node, A_Const)) @@ -1410,7 +1422,7 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, (errcode(ERRCODE_SYNTAX_ERROR), /* translator: %s is name of a SQL construct, eg ORDER BY */ errmsg("non-integer constant in %s", - clauseText[clause]), + ParseExprKindName(exprKind)), parser_errposition(pstate, location))); target_pos = intVal(val); @@ -1421,21 +1433,25 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, if (!tle->resjunk) { if (++targetlist_pos == target_pos) - return tle; /* return the unique match */ + { + /* return the unique match, after suitable validation */ + checkTargetlistEntrySQL92(pstate, tle, exprKind); + return tle; + } } } ereport(ERROR, (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), /* translator: %s is name of a SQL construct, eg ORDER BY */ errmsg("%s position %d is not in select list", - clauseText[clause], target_pos), + ParseExprKindName(exprKind), target_pos), parser_errposition(pstate, location))); } /* * Otherwise, we have an expression, so process it per SQL99 rules. */ - return findTargetlistEntrySQL99(pstate, node, tlist); + return findTargetlistEntrySQL99(pstate, node, tlist, exprKind); } /* @@ -1449,9 +1465,11 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, * * node the ORDER BY, GROUP BY, etc expression to be matched * tlist the target list (passed by reference so we can append to it) + * exprKind identifies clause type being processed */ static TargetEntry * -findTargetlistEntrySQL99(ParseState *pstate, Node *node, List **tlist) +findTargetlistEntrySQL99(ParseState *pstate, Node *node, List **tlist, + ParseExprKind exprKind) { TargetEntry *target_result; ListCell *tl; @@ -1464,7 +1482,7 @@ findTargetlistEntrySQL99(ParseState *pstate, Node *node, List **tlist) * resjunk target here, though the SQL92 cases above must ignore resjunk * targets. */ - expr = transformExpr(pstate, node); + expr = transformExpr(pstate, node, exprKind); foreach(tl, *tlist) { @@ -1491,7 +1509,8 @@ findTargetlistEntrySQL99(ParseState *pstate, Node *node, List **tlist) * end of the target list. This target is given resjunk = TRUE so that it * will not be projected into the final tuple. */ - target_result = transformTargetEntry(pstate, node, expr, NULL, true); + target_result = transformTargetEntry(pstate, node, expr, exprKind, + NULL, true); *tlist = lappend(*tlist, target_result); @@ -1511,7 +1530,7 @@ findTargetlistEntrySQL99(ParseState *pstate, Node *node, List **tlist) List * transformGroupClause(ParseState *pstate, List *grouplist, List **targetlist, List *sortClause, - bool useSQL99) + ParseExprKind exprKind, bool useSQL99) { List *result = NIL; ListCell *gl; @@ -1523,10 +1542,11 @@ transformGroupClause(ParseState *pstate, List *grouplist, bool found = false; if (useSQL99) - tle = findTargetlistEntrySQL99(pstate, gexpr, targetlist); + tle = findTargetlistEntrySQL99(pstate, gexpr, + targetlist, exprKind); else - tle = findTargetlistEntrySQL92(pstate, gexpr, targetlist, - GROUP_CLAUSE); + tle = findTargetlistEntrySQL92(pstate, gexpr, + targetlist, exprKind); /* Eliminate duplicates (GROUP BY x, x) */ if (targetIsInSortList(tle, InvalidOid, result)) @@ -1588,6 +1608,7 @@ List * transformSortClause(ParseState *pstate, List *orderlist, List **targetlist, + ParseExprKind exprKind, bool resolveUnknown, bool useSQL99) { @@ -1600,10 +1621,11 @@ transformSortClause(ParseState *pstate, TargetEntry *tle; if (useSQL99) - tle = findTargetlistEntrySQL99(pstate, sortby->node, targetlist); + tle = findTargetlistEntrySQL99(pstate, sortby->node, + targetlist, exprKind); else - tle = findTargetlistEntrySQL92(pstate, sortby->node, targetlist, - ORDER_CLAUSE); + tle = findTargetlistEntrySQL92(pstate, sortby->node, + targetlist, exprKind); sortlist = addTargetToSortList(pstate, tle, sortlist, *targetlist, sortby, @@ -1668,12 +1690,14 @@ transformWindowDefinitions(ParseState *pstate, orderClause = transformSortClause(pstate, windef->orderClause, targetlist, + EXPR_KIND_WINDOW_ORDER, true /* fix unknowns */ , true /* force SQL99 rules */ ); partitionClause = transformGroupClause(pstate, windef->partitionClause, targetlist, orderClause, + EXPR_KIND_WINDOW_PARTITION, true /* force SQL99 rules */ ); /* @@ -1861,7 +1885,7 @@ transformDistinctOnClause(ParseState *pstate, List *distinctlist, TargetEntry *tle; tle = findTargetlistEntrySQL92(pstate, dexpr, targetlist, - DISTINCT_ON_CLAUSE); + EXPR_KIND_DISTINCT_ON); sortgroupref = assignSortGroupRef(tle, *targetlist); sortgrouprefs = lappend_int(sortgrouprefs, sortgroupref); } @@ -2270,11 +2294,11 @@ transformFrameOffset(ParseState *pstate, int frameOptions, Node *clause) if (clause == NULL) return NULL; - /* Transform the raw expression tree */ - node = transformExpr(pstate, clause); - if (frameOptions & FRAMEOPTION_ROWS) { + /* Transform the raw expression tree */ + node = transformExpr(pstate, clause, EXPR_KIND_WINDOW_FRAME_ROWS); + /* * Like LIMIT clause, simply coerce to int8 */ @@ -2283,6 +2307,9 @@ transformFrameOffset(ParseState *pstate, int frameOptions, Node *clause) } else if (frameOptions & FRAMEOPTION_RANGE) { + /* Transform the raw expression tree */ + node = transformExpr(pstate, clause, EXPR_KIND_WINDOW_FRAME_RANGE); + /* * this needs a lot of thought to decide how to support in the context * of Postgres' extensible datatype framework @@ -2292,9 +2319,12 @@ transformFrameOffset(ParseState *pstate, int frameOptions, Node *clause) elog(ERROR, "window frame with value offset is not implemented"); } else + { Assert(false); + node = NULL; + } - /* Disallow variables and aggregates in frame offsets */ + /* Disallow variables in frame offsets */ checkExprIsVarFree(pstate, node, constructName); return node; diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 385f8e767e..e9267c56fc 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -37,6 +37,7 @@ bool Transform_null_equals = false; +static Node *transformExprRecurse(ParseState *pstate, Node *expr); static Node *transformParamRef(ParseState *pstate, ParamRef *pref); static Node *transformAExprOp(ParseState *pstate, A_Expr *a); static Node *transformAExprAnd(ParseState *pstate, A_Expr *a); @@ -100,9 +101,27 @@ static Expr *make_distinct_op(ParseState *pstate, List *opname, * input and output of transformExpr; see SubLink for example. */ Node * -transformExpr(ParseState *pstate, Node *expr) +transformExpr(ParseState *pstate, Node *expr, ParseExprKind exprKind) { - Node *result = NULL; + Node *result; + ParseExprKind sv_expr_kind; + + /* Save and restore identity of expression type we're parsing */ + Assert(exprKind != EXPR_KIND_NONE); + sv_expr_kind = pstate->p_expr_kind; + pstate->p_expr_kind = exprKind; + + result = transformExprRecurse(pstate, expr); + + pstate->p_expr_kind = sv_expr_kind; + + return result; +} + +static Node * +transformExprRecurse(ParseState *pstate, Node *expr) +{ + Node *result; if (expr == NULL) return NULL; @@ -133,7 +152,7 @@ transformExpr(ParseState *pstate, Node *expr) { A_Indirection *ind = (A_Indirection *) expr; - result = transformExpr(pstate, ind->arg); + result = transformExprRecurse(pstate, ind->arg); result = transformIndirection(pstate, result, ind->indirection); break; @@ -230,6 +249,8 @@ transformExpr(ParseState *pstate, Node *expr) break; default: elog(ERROR, "unrecognized A_Expr kind: %d", a->kind); + result = NULL; /* keep compiler quiet */ + break; } break; } @@ -242,7 +263,7 @@ transformExpr(ParseState *pstate, Node *expr) { NamedArgExpr *na = (NamedArgExpr *) expr; - na->arg = (Expr *) transformExpr(pstate, (Node *) na->arg); + na->arg = (Expr *) transformExprRecurse(pstate, (Node *) na->arg); result = expr; break; } @@ -279,7 +300,7 @@ transformExpr(ParseState *pstate, Node *expr) { NullTest *n = (NullTest *) expr; - n->arg = (Expr *) transformExpr(pstate, (Node *) n->arg); + n->arg = (Expr *) transformExprRecurse(pstate, (Node *) n->arg); /* the argument can be any type, so don't coerce it */ n->argisrow = type_is_rowtype(exprType((Node *) n->arg)); result = expr; @@ -334,6 +355,7 @@ transformExpr(ParseState *pstate, Node *expr) default: /* should not reach here */ elog(ERROR, "unrecognized node type: %d", (int) nodeTag(expr)); + result = NULL; /* keep compiler quiet */ break; } @@ -843,7 +865,7 @@ transformAExprOp(ParseState *pstate, A_Expr *a) else n->arg = (Expr *) lexpr; - result = transformExpr(pstate, (Node *) n); + result = transformExprRecurse(pstate, (Node *) n); } else if (lexpr && IsA(lexpr, RowExpr) && rexpr && IsA(rexpr, SubLink) && @@ -860,14 +882,14 @@ transformAExprOp(ParseState *pstate, A_Expr *a) s->testexpr = lexpr; s->operName = a->name; s->location = a->location; - result = transformExpr(pstate, (Node *) s); + result = transformExprRecurse(pstate, (Node *) s); } else if (lexpr && IsA(lexpr, RowExpr) && rexpr && IsA(rexpr, RowExpr)) { /* "row op row" */ - lexpr = transformExpr(pstate, lexpr); - rexpr = transformExpr(pstate, rexpr); + lexpr = transformExprRecurse(pstate, lexpr); + rexpr = transformExprRecurse(pstate, rexpr); Assert(IsA(lexpr, RowExpr)); Assert(IsA(rexpr, RowExpr)); @@ -880,8 +902,8 @@ transformAExprOp(ParseState *pstate, A_Expr *a) else { /* Ordinary scalar operator */ - lexpr = transformExpr(pstate, lexpr); - rexpr = transformExpr(pstate, rexpr); + lexpr = transformExprRecurse(pstate, lexpr); + rexpr = transformExprRecurse(pstate, rexpr); result = (Node *) make_op(pstate, a->name, @@ -896,8 +918,8 @@ transformAExprOp(ParseState *pstate, A_Expr *a) static Node * transformAExprAnd(ParseState *pstate, A_Expr *a) { - Node *lexpr = transformExpr(pstate, a->lexpr); - Node *rexpr = transformExpr(pstate, a->rexpr); + Node *lexpr = transformExprRecurse(pstate, a->lexpr); + Node *rexpr = transformExprRecurse(pstate, a->rexpr); lexpr = coerce_to_boolean(pstate, lexpr, "AND"); rexpr = coerce_to_boolean(pstate, rexpr, "AND"); @@ -910,8 +932,8 @@ transformAExprAnd(ParseState *pstate, A_Expr *a) static Node * transformAExprOr(ParseState *pstate, A_Expr *a) { - Node *lexpr = transformExpr(pstate, a->lexpr); - Node *rexpr = transformExpr(pstate, a->rexpr); + Node *lexpr = transformExprRecurse(pstate, a->lexpr); + Node *rexpr = transformExprRecurse(pstate, a->rexpr); lexpr = coerce_to_boolean(pstate, lexpr, "OR"); rexpr = coerce_to_boolean(pstate, rexpr, "OR"); @@ -924,7 +946,7 @@ transformAExprOr(ParseState *pstate, A_Expr *a) static Node * transformAExprNot(ParseState *pstate, A_Expr *a) { - Node *rexpr = transformExpr(pstate, a->rexpr); + Node *rexpr = transformExprRecurse(pstate, a->rexpr); rexpr = coerce_to_boolean(pstate, rexpr, "NOT"); @@ -936,8 +958,8 @@ transformAExprNot(ParseState *pstate, A_Expr *a) static Node * transformAExprOpAny(ParseState *pstate, A_Expr *a) { - Node *lexpr = transformExpr(pstate, a->lexpr); - Node *rexpr = transformExpr(pstate, a->rexpr); + Node *lexpr = transformExprRecurse(pstate, a->lexpr); + Node *rexpr = transformExprRecurse(pstate, a->rexpr); return (Node *) make_scalar_array_op(pstate, a->name, @@ -950,8 +972,8 @@ transformAExprOpAny(ParseState *pstate, A_Expr *a) static Node * transformAExprOpAll(ParseState *pstate, A_Expr *a) { - Node *lexpr = transformExpr(pstate, a->lexpr); - Node *rexpr = transformExpr(pstate, a->rexpr); + Node *lexpr = transformExprRecurse(pstate, a->lexpr); + Node *rexpr = transformExprRecurse(pstate, a->rexpr); return (Node *) make_scalar_array_op(pstate, a->name, @@ -964,8 +986,8 @@ transformAExprOpAll(ParseState *pstate, A_Expr *a) static Node * transformAExprDistinct(ParseState *pstate, A_Expr *a) { - Node *lexpr = transformExpr(pstate, a->lexpr); - Node *rexpr = transformExpr(pstate, a->rexpr); + Node *lexpr = transformExprRecurse(pstate, a->lexpr); + Node *rexpr = transformExprRecurse(pstate, a->rexpr); if (lexpr && IsA(lexpr, RowExpr) && rexpr && IsA(rexpr, RowExpr)) @@ -990,8 +1012,8 @@ transformAExprDistinct(ParseState *pstate, A_Expr *a) static Node * transformAExprNullIf(ParseState *pstate, A_Expr *a) { - Node *lexpr = transformExpr(pstate, a->lexpr); - Node *rexpr = transformExpr(pstate, a->rexpr); + Node *lexpr = transformExprRecurse(pstate, a->lexpr); + Node *rexpr = transformExprRecurse(pstate, a->rexpr); OpExpr *result; result = (OpExpr *) make_op(pstate, @@ -1029,7 +1051,7 @@ transformAExprOf(ParseState *pstate, A_Expr *a) * Checking an expression for match to a list of type names. Will result * in a boolean constant node. */ - Node *lexpr = transformExpr(pstate, a->lexpr); + Node *lexpr = transformExprRecurse(pstate, a->lexpr); Const *result; ListCell *telem; Oid ltype, @@ -1092,12 +1114,12 @@ transformAExprIn(ParseState *pstate, A_Expr *a) * First step: transform all the inputs, and detect whether any are * RowExprs or contain Vars. */ - lexpr = transformExpr(pstate, a->lexpr); + lexpr = transformExprRecurse(pstate, a->lexpr); haveRowExpr = (lexpr && IsA(lexpr, RowExpr)); rexprs = rvars = rnonvars = NIL; foreach(l, (List *) a->rexpr) { - Node *rexpr = transformExpr(pstate, lfirst(l)); + Node *rexpr = transformExprRecurse(pstate, lfirst(l)); haveRowExpr |= (rexpr && IsA(rexpr, RowExpr)); rexprs = lappend(rexprs, rexpr); @@ -1222,8 +1244,8 @@ transformFuncCall(ParseState *pstate, FuncCall *fn) targs = NIL; foreach(args, fn->args) { - targs = lappend(targs, transformExpr(pstate, - (Node *) lfirst(args))); + targs = lappend(targs, transformExprRecurse(pstate, + (Node *) lfirst(args))); } /* ... and hand off to ParseFuncOrColumn */ @@ -1258,7 +1280,7 @@ transformCaseExpr(ParseState *pstate, CaseExpr *c) newc = makeNode(CaseExpr); /* transform the test expression, if any */ - arg = transformExpr(pstate, (Node *) c->arg); + arg = transformExprRecurse(pstate, (Node *) c->arg); /* generate placeholder for test expression */ if (arg) @@ -1311,14 +1333,14 @@ transformCaseExpr(ParseState *pstate, CaseExpr *c) warg, w->location); } - neww->expr = (Expr *) transformExpr(pstate, warg); + neww->expr = (Expr *) transformExprRecurse(pstate, warg); neww->expr = (Expr *) coerce_to_boolean(pstate, (Node *) neww->expr, "CASE/WHEN"); warg = (Node *) w->result; - neww->result = (Expr *) transformExpr(pstate, warg); + neww->result = (Expr *) transformExprRecurse(pstate, warg); neww->location = w->location; newargs = lappend(newargs, neww); @@ -1337,7 +1359,7 @@ transformCaseExpr(ParseState *pstate, CaseExpr *c) n->location = -1; defresult = (Node *) n; } - newc->defresult = (Expr *) transformExpr(pstate, defresult); + newc->defresult = (Expr *) transformExprRecurse(pstate, defresult); /* * Note: default result is considered the most significant type in @@ -1380,12 +1402,92 @@ transformSubLink(ParseState *pstate, SubLink *sublink) { Node *result = (Node *) sublink; Query *qtree; + const char *err; /* If we already transformed this node, do nothing */ if (IsA(sublink->subselect, Query)) return result; + /* + * Check to see if the sublink is in an invalid place within the query. + * We allow sublinks everywhere in SELECT/INSERT/UPDATE/DELETE, but + * generally not in utility statements. + */ + err = NULL; + switch (pstate->p_expr_kind) + { + case EXPR_KIND_NONE: + Assert(false); /* can't happen */ + break; + case EXPR_KIND_OTHER: + /* Accept sublink here; caller must throw error if wanted */ + break; + case EXPR_KIND_JOIN_ON: + case EXPR_KIND_JOIN_USING: + case EXPR_KIND_FROM_SUBSELECT: + case EXPR_KIND_FROM_FUNCTION: + case EXPR_KIND_WHERE: + case EXPR_KIND_HAVING: + case EXPR_KIND_WINDOW_PARTITION: + case EXPR_KIND_WINDOW_ORDER: + case EXPR_KIND_WINDOW_FRAME_RANGE: + case EXPR_KIND_WINDOW_FRAME_ROWS: + case EXPR_KIND_SELECT_TARGET: + case EXPR_KIND_INSERT_TARGET: + case EXPR_KIND_UPDATE_SOURCE: + case EXPR_KIND_UPDATE_TARGET: + case EXPR_KIND_GROUP_BY: + case EXPR_KIND_ORDER_BY: + case EXPR_KIND_DISTINCT_ON: + case EXPR_KIND_LIMIT: + case EXPR_KIND_OFFSET: + case EXPR_KIND_RETURNING: + case EXPR_KIND_VALUES: + /* okay */ + break; + case EXPR_KIND_CHECK_CONSTRAINT: + case EXPR_KIND_DOMAIN_CHECK: + err = _("cannot use subquery in CHECK constraint"); + break; + case EXPR_KIND_COLUMN_DEFAULT: + case EXPR_KIND_FUNCTION_DEFAULT: + err = _("cannot use subquery in DEFAULT expression"); + break; + case EXPR_KIND_INDEX_EXPRESSION: + err = _("cannot use subquery in index expression"); + break; + case EXPR_KIND_INDEX_PREDICATE: + err = _("cannot use subquery in index predicate"); + break; + case EXPR_KIND_ALTER_COL_TRANSFORM: + err = _("cannot use subquery in transform expression"); + break; + case EXPR_KIND_EXECUTE_PARAMETER: + err = _("cannot use subquery in EXECUTE parameter"); + break; + case EXPR_KIND_TRIGGER_WHEN: + err = _("cannot use subquery in trigger WHEN condition"); + break; + + /* + * There is intentionally no default: case here, so that the + * compiler will warn if we add a new ParseExprKind without + * extending this switch. If we do see an unrecognized value at + * runtime, the behavior will be the same as for EXPR_KIND_OTHER, + * which is sane anyway. + */ + } + if (err) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg_internal("%s", err), + parser_errposition(pstate, sublink->location))); + pstate->p_hasSubLinks = true; + + /* + * OK, let's transform the sub-SELECT. + */ qtree = parse_sub_analyze(sublink->subselect, pstate, NULL, false); /* @@ -1450,7 +1552,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink) /* * Transform lefthand expression, and convert to a list */ - lefthand = transformExpr(pstate, sublink->testexpr); + lefthand = transformExprRecurse(pstate, sublink->testexpr); if (lefthand && IsA(lefthand, RowExpr)) left_list = ((RowExpr *) lefthand)->args; else @@ -1557,7 +1659,7 @@ transformArrayExpr(ParseState *pstate, A_ArrayExpr *a, } else { - newe = transformExpr(pstate, e); + newe = transformExprRecurse(pstate, e); /* * Check for sub-array expressions, if we haven't already found @@ -1679,7 +1781,7 @@ transformRowExpr(ParseState *pstate, RowExpr *r) ListCell *lc; /* Transform the field expressions */ - newr->args = transformExpressionList(pstate, r->args); + newr->args = transformExpressionList(pstate, r->args, pstate->p_expr_kind); /* Barring later casting, we consider the type RECORD */ newr->row_typeid = RECORDOID; @@ -1712,7 +1814,7 @@ transformCoalesceExpr(ParseState *pstate, CoalesceExpr *c) Node *e = (Node *) lfirst(args); Node *newe; - newe = transformExpr(pstate, e); + newe = transformExprRecurse(pstate, e); newargs = lappend(newargs, newe); } @@ -1751,7 +1853,7 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m) Node *e = (Node *) lfirst(args); Node *newe; - newe = transformExpr(pstate, e); + newe = transformExprRecurse(pstate, e); newargs = lappend(newargs, newe); } @@ -1805,7 +1907,7 @@ transformXmlExpr(ParseState *pstate, XmlExpr *x) Assert(IsA(r, ResTarget)); - expr = transformExpr(pstate, r->val); + expr = transformExprRecurse(pstate, r->val); if (r->name) argname = map_sql_identifier_to_xml_name(r->name, false, false); @@ -1851,7 +1953,7 @@ transformXmlExpr(ParseState *pstate, XmlExpr *x) Node *e = (Node *) lfirst(lc); Node *newe; - newe = transformExpr(pstate, e); + newe = transformExprRecurse(pstate, e); switch (x->op) { case IS_XMLCONCAT: @@ -1914,7 +2016,7 @@ transformXmlSerialize(ParseState *pstate, XmlSerialize *xs) xexpr = makeNode(XmlExpr); xexpr->op = IS_XMLSERIALIZE; xexpr->args = list_make1(coerce_to_specific_type(pstate, - transformExpr(pstate, xs->expr), + transformExprRecurse(pstate, xs->expr), XMLOID, "XMLSERIALIZE")); @@ -1977,7 +2079,7 @@ transformBooleanTest(ParseState *pstate, BooleanTest *b) clausename = NULL; /* keep compiler quiet */ } - b->arg = (Expr *) transformExpr(pstate, (Node *) b->arg); + b->arg = (Expr *) transformExprRecurse(pstate, (Node *) b->arg); b->arg = (Expr *) coerce_to_boolean(pstate, (Node *) b->arg, @@ -2082,7 +2184,7 @@ static Node * transformTypeCast(ParseState *pstate, TypeCast *tc) { Node *result; - Node *expr = transformExpr(pstate, tc->arg); + Node *expr = transformExprRecurse(pstate, tc->arg); Oid inputType = exprType(expr); Oid targetType; int32 targetTypmod; @@ -2130,7 +2232,7 @@ transformCollateClause(ParseState *pstate, CollateClause *c) Oid argtype; newc = makeNode(CollateExpr); - newc->arg = (Expr *) transformExpr(pstate, c->arg); + newc->arg = (Expr *) transformExprRecurse(pstate, c->arg); argtype = exprType((Node *) newc->arg); @@ -2433,3 +2535,87 @@ make_distinct_op(ParseState *pstate, List *opname, Node *ltree, Node *rtree, return result; } + +/* + * Produce a string identifying an expression by kind. + * + * Note: when practical, use a simple SQL keyword for the result. If that + * doesn't work well, check call sites to see whether custom error message + * strings are required. + */ +const char * +ParseExprKindName(ParseExprKind exprKind) +{ + switch (exprKind) + { + case EXPR_KIND_NONE: + return "invalid expression context"; + case EXPR_KIND_OTHER: + return "extension expression"; + case EXPR_KIND_JOIN_ON: + return "JOIN/ON"; + case EXPR_KIND_JOIN_USING: + return "JOIN/USING"; + case EXPR_KIND_FROM_SUBSELECT: + return "sub-SELECT in FROM"; + case EXPR_KIND_FROM_FUNCTION: + return "function in FROM"; + case EXPR_KIND_WHERE: + return "WHERE"; + case EXPR_KIND_HAVING: + return "HAVING"; + case EXPR_KIND_WINDOW_PARTITION: + return "window PARTITION BY"; + case EXPR_KIND_WINDOW_ORDER: + return "window ORDER BY"; + case EXPR_KIND_WINDOW_FRAME_RANGE: + return "window RANGE"; + case EXPR_KIND_WINDOW_FRAME_ROWS: + return "window ROWS"; + case EXPR_KIND_SELECT_TARGET: + return "SELECT"; + case EXPR_KIND_INSERT_TARGET: + return "INSERT"; + case EXPR_KIND_UPDATE_SOURCE: + case EXPR_KIND_UPDATE_TARGET: + return "UPDATE"; + case EXPR_KIND_GROUP_BY: + return "GROUP BY"; + case EXPR_KIND_ORDER_BY: + return "ORDER BY"; + case EXPR_KIND_DISTINCT_ON: + return "DISTINCT ON"; + case EXPR_KIND_LIMIT: + return "LIMIT"; + case EXPR_KIND_OFFSET: + return "OFFSET"; + case EXPR_KIND_RETURNING: + return "RETURNING"; + case EXPR_KIND_VALUES: + return "VALUES"; + case EXPR_KIND_CHECK_CONSTRAINT: + case EXPR_KIND_DOMAIN_CHECK: + return "CHECK"; + case EXPR_KIND_COLUMN_DEFAULT: + case EXPR_KIND_FUNCTION_DEFAULT: + return "DEFAULT"; + case EXPR_KIND_INDEX_EXPRESSION: + return "index expression"; + case EXPR_KIND_INDEX_PREDICATE: + return "index predicate"; + case EXPR_KIND_ALTER_COL_TRANSFORM: + return "USING"; + case EXPR_KIND_EXECUTE_PARAMETER: + return "EXECUTE"; + case EXPR_KIND_TRIGGER_WHEN: + return "WHEN"; + + /* + * There is intentionally no default: case here, so that the + * compiler will warn if we add a new ParseExprKind without + * extending this switch. If we do see an unrecognized value at + * runtime, we'll fall through to the "unrecognized" return. + */ + } + return "unrecognized expression kind"; +} diff --git a/src/backend/parser/parse_node.c b/src/backend/parser/parse_node.c index 80dbdd19e4..6aaeae76b0 100644 --- a/src/backend/parser/parse_node.c +++ b/src/backend/parser/parse_node.c @@ -328,7 +328,7 @@ transformArraySubscripts(ParseState *pstate, { if (ai->lidx) { - subexpr = transformExpr(pstate, ai->lidx); + subexpr = transformExpr(pstate, ai->lidx, pstate->p_expr_kind); /* If it's not int4 already, try to coerce */ subexpr = coerce_to_target_type(pstate, subexpr, exprType(subexpr), @@ -355,7 +355,7 @@ transformArraySubscripts(ParseState *pstate, } lowerIndexpr = lappend(lowerIndexpr, subexpr); } - subexpr = transformExpr(pstate, ai->uidx); + subexpr = transformExpr(pstate, ai->uidx, pstate->p_expr_kind); /* If it's not int4 already, try to coerce */ subexpr = coerce_to_target_type(pstate, subexpr, exprType(subexpr), diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c index ccd97fc845..053b3a0dad 100644 --- a/src/backend/parser/parse_target.c +++ b/src/backend/parser/parse_target.c @@ -57,14 +57,14 @@ static Node *transformAssignmentSubscripts(ParseState *pstate, Node *rhs, int location); static List *ExpandColumnRefStar(ParseState *pstate, ColumnRef *cref, - bool targetlist); + bool make_target_entry); static List *ExpandAllTables(ParseState *pstate, int location); static List *ExpandIndirectionStar(ParseState *pstate, A_Indirection *ind, - bool targetlist); + bool make_target_entry, ParseExprKind exprKind); static List *ExpandSingleTable(ParseState *pstate, RangeTblEntry *rte, - int location, bool targetlist); + int location, bool make_target_entry); static List *ExpandRowReference(ParseState *pstate, Node *expr, - bool targetlist); + bool make_target_entry); static int FigureColnameInternal(Node *node, char **name); @@ -76,6 +76,7 @@ static int FigureColnameInternal(Node *node, char **name); * * node the (untransformed) parse tree for the value expression. * expr the transformed expression, or NULL if caller didn't do it yet. + * exprKind expression kind (EXPR_KIND_SELECT_TARGET, etc) * colname the column name to be assigned, or NULL if none yet set. * resjunk true if the target should be marked resjunk, ie, it is not * wanted in the final projected tuple. @@ -84,12 +85,13 @@ TargetEntry * transformTargetEntry(ParseState *pstate, Node *node, Node *expr, + ParseExprKind exprKind, char *colname, bool resjunk) { /* Transform the node if caller didn't do it already */ if (expr == NULL) - expr = transformExpr(pstate, node); + expr = transformExpr(pstate, node, exprKind); if (colname == NULL && !resjunk) { @@ -111,11 +113,13 @@ transformTargetEntry(ParseState *pstate, * transformTargetList() * Turns a list of ResTarget's into a list of TargetEntry's. * - * At this point, we don't care whether we are doing SELECT, INSERT, - * or UPDATE; we just transform the given expressions (the "val" fields). + * At this point, we don't care whether we are doing SELECT, UPDATE, + * or RETURNING; we just transform the given expressions (the "val" fields). + * However, our subroutines care, so we need the exprKind parameter. */ List * -transformTargetList(ParseState *pstate, List *targetlist) +transformTargetList(ParseState *pstate, List *targetlist, + ParseExprKind exprKind) { List *p_target = NIL; ListCell *o_target; @@ -151,7 +155,7 @@ transformTargetList(ParseState *pstate, List *targetlist) /* It is something.*, expand into multiple items */ p_target = list_concat(p_target, ExpandIndirectionStar(pstate, ind, - true)); + true, exprKind)); continue; } } @@ -163,6 +167,7 @@ transformTargetList(ParseState *pstate, List *targetlist) transformTargetEntry(pstate, res->val, NULL, + exprKind, res->name, false)); } @@ -180,7 +185,8 @@ transformTargetList(ParseState *pstate, List *targetlist) * decoration. We use this for ROW() and VALUES() constructs. */ List * -transformExpressionList(ParseState *pstate, List *exprlist) +transformExpressionList(ParseState *pstate, List *exprlist, + ParseExprKind exprKind) { List *result = NIL; ListCell *lc; @@ -216,7 +222,7 @@ transformExpressionList(ParseState *pstate, List *exprlist) /* It is something.*, expand into multiple items */ result = list_concat(result, ExpandIndirectionStar(pstate, ind, - false)); + false, exprKind)); continue; } } @@ -225,7 +231,7 @@ transformExpressionList(ParseState *pstate, List *exprlist) * Not "something.*", so transform as a single expression */ result = lappend(result, - transformExpr(pstate, e)); + transformExpr(pstate, e, exprKind)); } return result; @@ -350,6 +356,7 @@ markTargetListOrigin(ParseState *pstate, TargetEntry *tle, * * pstate parse state * expr expression to be modified + * exprKind indicates which type of statement we're dealing with * colname target column name (ie, name of attribute to be assigned to) * attrno target attribute number * indirection subscripts/field names for target column, if any @@ -365,16 +372,27 @@ markTargetListOrigin(ParseState *pstate, TargetEntry *tle, Expr * transformAssignedExpr(ParseState *pstate, Expr *expr, + ParseExprKind exprKind, char *colname, int attrno, List *indirection, int location) { + Relation rd = pstate->p_target_relation; Oid type_id; /* type of value provided */ Oid attrtype; /* type of target column */ int32 attrtypmod; Oid attrcollation; /* collation of target column */ - Relation rd = pstate->p_target_relation; + ParseExprKind sv_expr_kind; + + /* + * Save and restore identity of expression type we're parsing. We must + * set p_expr_kind here because we can parse subscripts without going + * through transformExpr(). + */ + Assert(exprKind != EXPR_KIND_NONE); + sv_expr_kind = pstate->p_expr_kind; + pstate->p_expr_kind = exprKind; Assert(rd != NULL); if (attrno <= 0) @@ -491,6 +509,8 @@ transformAssignedExpr(ParseState *pstate, parser_errposition(pstate, exprLocation(orig_expr)))); } + pstate->p_expr_kind = sv_expr_kind; + return expr; } @@ -521,6 +541,7 @@ updateTargetListEntry(ParseState *pstate, /* Fix up expression as needed */ tle->expr = transformAssignedExpr(pstate, tle->expr, + EXPR_KIND_UPDATE_TARGET, colname, attrno, indirection, @@ -947,7 +968,7 @@ checkInsertTargets(ParseState *pstate, List *cols, List **attrnos) */ static List * ExpandColumnRefStar(ParseState *pstate, ColumnRef *cref, - bool targetlist) + bool make_target_entry) { List *fields = cref->fields; int numnames = list_length(fields); @@ -960,9 +981,9 @@ ExpandColumnRefStar(ParseState *pstate, ColumnRef *cref, * (e.g., SELECT * FROM emp, dept) * * Since the grammar only accepts bare '*' at top level of SELECT, we - * need not handle the targetlist==false case here. + * need not handle the make_target_entry==false case here. */ - Assert(targetlist); + Assert(make_target_entry); return ExpandAllTables(pstate, cref->location); } else @@ -1002,7 +1023,7 @@ ExpandColumnRefStar(ParseState *pstate, ColumnRef *cref, node = (*pstate->p_pre_columnref_hook) (pstate, cref); if (node != NULL) - return ExpandRowReference(pstate, node, targetlist); + return ExpandRowReference(pstate, node, make_target_entry); } switch (numnames) @@ -1065,7 +1086,7 @@ ExpandColumnRefStar(ParseState *pstate, ColumnRef *cref, errmsg("column reference \"%s\" is ambiguous", NameListToString(cref->fields)), parser_errposition(pstate, cref->location))); - return ExpandRowReference(pstate, node, targetlist); + return ExpandRowReference(pstate, node, make_target_entry); } } @@ -1100,7 +1121,7 @@ ExpandColumnRefStar(ParseState *pstate, ColumnRef *cref, /* * OK, expand the RTE into fields. */ - return ExpandSingleTable(pstate, rte, cref->location, targetlist); + return ExpandSingleTable(pstate, rte, cref->location, make_target_entry); } } @@ -1166,10 +1187,12 @@ ExpandAllTables(ParseState *pstate, int location) * The code is shared between the case of foo.* at the top level in a SELECT * target list (where we want TargetEntry nodes in the result) and foo.* in * a ROW() or VALUES() construct (where we want just bare expressions). + * For robustness, we use a separate "make_target_entry" flag to control + * this rather than relying on exprKind. */ static List * ExpandIndirectionStar(ParseState *pstate, A_Indirection *ind, - bool targetlist) + bool make_target_entry, ParseExprKind exprKind) { Node *expr; @@ -1179,10 +1202,10 @@ ExpandIndirectionStar(ParseState *pstate, A_Indirection *ind, list_length(ind->indirection) - 1); /* And transform that */ - expr = transformExpr(pstate, (Node *) ind); + expr = transformExpr(pstate, (Node *) ind, exprKind); /* Expand the rowtype expression into individual fields */ - return ExpandRowReference(pstate, expr, targetlist); + return ExpandRowReference(pstate, expr, make_target_entry); } /* @@ -1196,14 +1219,14 @@ ExpandIndirectionStar(ParseState *pstate, A_Indirection *ind, */ static List * ExpandSingleTable(ParseState *pstate, RangeTblEntry *rte, - int location, bool targetlist) + int location, bool make_target_entry) { int sublevels_up; int rtindex; rtindex = RTERangeTablePosn(pstate, rte, &sublevels_up); - if (targetlist) + if (make_target_entry) { /* expandRelAttrs handles permissions marking */ return expandRelAttrs(pstate, rte, rtindex, sublevels_up, @@ -1245,7 +1268,7 @@ ExpandSingleTable(ParseState *pstate, RangeTblEntry *rte, */ static List * ExpandRowReference(ParseState *pstate, Node *expr, - bool targetlist) + bool make_target_entry) { List *result = NIL; TupleDesc tupleDesc; @@ -1268,7 +1291,7 @@ ExpandRowReference(ParseState *pstate, Node *expr, RangeTblEntry *rte; rte = GetRTEByRangeTablePosn(pstate, var->varno, var->varlevelsup); - return ExpandSingleTable(pstate, rte, var->location, targetlist); + return ExpandSingleTable(pstate, rte, var->location, make_target_entry); } /* @@ -1313,7 +1336,7 @@ ExpandRowReference(ParseState *pstate, Node *expr, /* save attribute's collation for parse_collate.c */ fselect->resultcollid = att->attcollation; - if (targetlist) + if (make_target_entry) { /* add TargetEntry decoration */ TargetEntry *te; diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index c22c6ed21f..accda01f45 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -1917,6 +1917,7 @@ transformIndexStmt(IndexStmt *stmt, const char *queryString) { stmt->whereClause = transformWhereClause(pstate, stmt->whereClause, + EXPR_KIND_INDEX_PREDICATE, "WHERE"); /* we have to fix its collations too */ assign_expr_collations(pstate, stmt->whereClause); @@ -1934,15 +1935,20 @@ transformIndexStmt(IndexStmt *stmt, const char *queryString) ielem->indexcolname = FigureIndexColname(ielem->expr); /* Now do parse transformation of the expression */ - ielem->expr = transformExpr(pstate, ielem->expr); + ielem->expr = transformExpr(pstate, ielem->expr, + EXPR_KIND_INDEX_EXPRESSION); /* We have to fix its collations too */ assign_expr_collations(pstate, ielem->expr); /* - * We check only that the result type is legitimate; this is for - * consistency with what transformWhereClause() checks for the - * predicate. DefineIndex() will make more checks. + * transformExpr() should have already rejected subqueries, + * aggregates, and window functions, based on the EXPR_KIND_ for + * an index expression. + * + * Also reject expressions returning sets; this is for consistency + * with what transformWhereClause() checks for the predicate. + * DefineIndex() will make more checks. */ if (expression_returns_set(ielem->expr)) ereport(ERROR, @@ -1952,7 +1958,8 @@ transformIndexStmt(IndexStmt *stmt, const char *queryString) } /* - * Check that only the base rel is mentioned. + * Check that only the base rel is mentioned. (This should be dead code + * now that add_missing_from is history.) */ if (list_length(pstate->p_rtable) != 1) ereport(ERROR, @@ -2047,25 +2054,17 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString, /* take care of the where clause */ *whereClause = transformWhereClause(pstate, (Node *) copyObject(stmt->whereClause), + EXPR_KIND_WHERE, "WHERE"); /* we have to fix its collations too */ assign_expr_collations(pstate, *whereClause); + /* this is probably dead code without add_missing_from: */ if (list_length(pstate->p_rtable) != 2) /* naughty, naughty... */ ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("rule WHERE condition cannot contain references to other relations"))); - /* aggregates not allowed (but subselects are okay) */ - if (pstate->p_hasAggs) - ereport(ERROR, - (errcode(ERRCODE_GROUPING_ERROR), - errmsg("cannot use aggregate function in rule WHERE condition"))); - if (pstate->p_hasWindowFuncs) - ereport(ERROR, - (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot use window function in rule WHERE condition"))); - /* * 'instead nothing' rules with a qualification need a query rangetable so * the rewrite handler can add the negated rule qualification to the diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index 9c778efd1c..5fcf274198 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -52,17 +52,6 @@ static Relids offset_relid_set(Relids relids, int offset); static Relids adjust_relid_set(Relids relids, int oldrelid, int newrelid); -/* - * checkExprHasAggs - - * Check if an expression contains an aggregate function call of the - * current query level. - */ -bool -checkExprHasAggs(Node *node) -{ - return contain_aggs_of_level(node, 0); -} - /* * contain_aggs_of_level - * Check if an expression contains an aggregate function call of a @@ -185,12 +174,12 @@ locate_agg_of_level_walker(Node *node, } /* - * checkExprHasWindowFuncs - + * contain_windowfuncs - * Check if an expression contains a window function call of the * current query level. */ bool -checkExprHasWindowFuncs(Node *node) +contain_windowfuncs(Node *node) { /* * Must be prepared to start with a Query or a bare expression tree; if @@ -1049,7 +1038,7 @@ AddQual(Query *parsetree, Node *qual) /* * We had better not have stuck an aggregate into the WHERE clause. */ - Assert(!checkExprHasAggs(copy)); + Assert(!contain_aggs_of_level(copy, 0)); /* * Make sure query is marked correctly if added qual has sublinks. Need diff --git a/src/include/optimizer/var.h b/src/include/optimizer/var.h index ec21df3a7e..e3ba3144f3 100644 --- a/src/include/optimizer/var.h +++ b/src/include/optimizer/var.h @@ -37,7 +37,6 @@ extern List *pull_vars_of_level(Node *node, int levelsup); extern bool contain_var_clause(Node *node); extern bool contain_vars_of_level(Node *node, int levelsup); extern int locate_var_of_level(Node *node, int levelsup); -extern int find_minimum_var_level(Node *node); extern List *pull_var_clause(Node *node, PVCAggregateBehavior aggbehavior, PVCPlaceHolderBehavior phbehavior); extern Node *flatten_join_alias_vars(PlannerInfo *root, Node *node); diff --git a/src/include/parser/parse_agg.h b/src/include/parser/parse_agg.h index b32ee6c272..c51fdd8141 100644 --- a/src/include/parser/parse_agg.h +++ b/src/include/parser/parse_agg.h @@ -22,7 +22,6 @@ extern void transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc, WindowDef *windef); extern void parseCheckAggregates(ParseState *pstate, Query *qry); -extern void parseCheckWindowFuncs(ParseState *pstate, Query *qry); extern void build_aggregate_fnexprs(Oid *agg_input_types, int agg_num_inputs, diff --git a/src/include/parser/parse_clause.h b/src/include/parser/parse_clause.h index fd3fc8f570..5d59ee973c 100644 --- a/src/include/parser/parse_clause.h +++ b/src/include/parser/parse_clause.h @@ -23,14 +23,15 @@ extern bool interpretInhOption(InhOption inhOpt); extern bool interpretOidsOption(List *defList); extern Node *transformWhereClause(ParseState *pstate, Node *clause, - const char *constructName); + ParseExprKind exprKind, const char *constructName); extern Node *transformLimitClause(ParseState *pstate, Node *clause, - const char *constructName); + ParseExprKind exprKind, const char *constructName); extern List *transformGroupClause(ParseState *pstate, List *grouplist, List **targetlist, List *sortClause, - bool useSQL99); + ParseExprKind exprKind, bool useSQL99); extern List *transformSortClause(ParseState *pstate, List *orderlist, - List **targetlist, bool resolveUnknown, bool useSQL99); + List **targetlist, ParseExprKind exprKind, + bool resolveUnknown, bool useSQL99); extern List *transformWindowDefinitions(ParseState *pstate, List *windowdefs, diff --git a/src/include/parser/parse_expr.h b/src/include/parser/parse_expr.h index cbf281e315..b9b9c7ee4d 100644 --- a/src/include/parser/parse_expr.h +++ b/src/include/parser/parse_expr.h @@ -18,6 +18,8 @@ /* GUC parameters */ extern bool Transform_null_equals; -extern Node *transformExpr(ParseState *pstate, Node *expr); +extern Node *transformExpr(ParseState *pstate, Node *expr, ParseExprKind exprKind); + +extern const char *ParseExprKindName(ParseExprKind exprKind); #endif /* PARSE_EXPR_H */ diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h index 200b9744e5..e3bb35f130 100644 --- a/src/include/parser/parse_node.h +++ b/src/include/parser/parse_node.h @@ -18,6 +18,54 @@ #include "utils/relcache.h" +/* + * Expression kinds distinguished by transformExpr(). Many of these are not + * semantically distinct so far as expression transformation goes; rather, + * we distinguish them so that context-specific error messages can be printed. + * + * Note: EXPR_KIND_OTHER is not used in the core code, but is left for use + * by extension code that might need to call transformExpr(). The core code + * will not enforce any context-driven restrictions on EXPR_KIND_OTHER + * expressions, so the caller would have to check for sub-selects, aggregates, + * and window functions if those need to be disallowed. + */ +typedef enum ParseExprKind +{ + EXPR_KIND_NONE = 0, /* "not in an expression" */ + EXPR_KIND_OTHER, /* reserved for extensions */ + EXPR_KIND_JOIN_ON, /* JOIN ON */ + EXPR_KIND_JOIN_USING, /* JOIN USING */ + EXPR_KIND_FROM_SUBSELECT, /* sub-SELECT in FROM clause */ + EXPR_KIND_FROM_FUNCTION, /* function in FROM clause */ + EXPR_KIND_WHERE, /* WHERE */ + EXPR_KIND_HAVING, /* HAVING */ + EXPR_KIND_WINDOW_PARTITION, /* window definition PARTITION BY */ + EXPR_KIND_WINDOW_ORDER, /* window definition ORDER BY */ + EXPR_KIND_WINDOW_FRAME_RANGE, /* window frame clause with RANGE */ + EXPR_KIND_WINDOW_FRAME_ROWS, /* window frame clause with ROWS */ + EXPR_KIND_SELECT_TARGET, /* SELECT target list item */ + EXPR_KIND_INSERT_TARGET, /* INSERT target list item */ + EXPR_KIND_UPDATE_SOURCE, /* UPDATE assignment source item */ + EXPR_KIND_UPDATE_TARGET, /* UPDATE assignment target item */ + EXPR_KIND_GROUP_BY, /* GROUP BY */ + EXPR_KIND_ORDER_BY, /* ORDER BY */ + EXPR_KIND_DISTINCT_ON, /* DISTINCT ON */ + EXPR_KIND_LIMIT, /* LIMIT */ + EXPR_KIND_OFFSET, /* OFFSET */ + EXPR_KIND_RETURNING, /* RETURNING */ + EXPR_KIND_VALUES, /* VALUES */ + EXPR_KIND_CHECK_CONSTRAINT, /* CHECK constraint for a table */ + EXPR_KIND_DOMAIN_CHECK, /* CHECK constraint for a domain */ + EXPR_KIND_COLUMN_DEFAULT, /* default value for a table column */ + EXPR_KIND_FUNCTION_DEFAULT, /* default parameter value for function */ + EXPR_KIND_INDEX_EXPRESSION, /* index expression */ + EXPR_KIND_INDEX_PREDICATE, /* index predicate */ + EXPR_KIND_ALTER_COL_TRANSFORM, /* transform expr in ALTER COLUMN TYPE */ + EXPR_KIND_EXECUTE_PARAMETER, /* parameter value in EXECUTE */ + EXPR_KIND_TRIGGER_WHEN /* WHEN condition in CREATE TRIGGER */ +} ParseExprKind; + + /* * Function signatures for parser hooks */ @@ -93,6 +141,7 @@ struct ParseState List *p_future_ctes; /* common table exprs not yet in namespace */ CommonTableExpr *p_parent_cte; /* this query's containing CTE */ List *p_windowdefs; /* raw representations of window clauses */ + ParseExprKind p_expr_kind; /* what kind of expression we're parsing */ int p_next_resno; /* next targetlist resno to assign */ List *p_locking_clause; /* raw FOR UPDATE/FOR SHARE info */ Node *p_value_substitute; /* what to replace VALUE with, if any */ diff --git a/src/include/parser/parse_target.h b/src/include/parser/parse_target.h index d274a66b13..e5bbaf4e12 100644 --- a/src/include/parser/parse_target.h +++ b/src/include/parser/parse_target.h @@ -17,13 +17,16 @@ #include "parser/parse_node.h" -extern List *transformTargetList(ParseState *pstate, List *targetlist); -extern List *transformExpressionList(ParseState *pstate, List *exprlist); +extern List *transformTargetList(ParseState *pstate, List *targetlist, + ParseExprKind exprKind); +extern List *transformExpressionList(ParseState *pstate, List *exprlist, + ParseExprKind exprKind); extern void markTargetListOrigins(ParseState *pstate, List *targetlist); extern TargetEntry *transformTargetEntry(ParseState *pstate, - Node *node, Node *expr, + Node *node, Node *expr, ParseExprKind exprKind, char *colname, bool resjunk); extern Expr *transformAssignedExpr(ParseState *pstate, Expr *expr, + ParseExprKind exprKind, char *colname, int attrno, List *indirection, diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h index 6f57b37b81..e13331dcb5 100644 --- a/src/include/rewrite/rewriteManip.h +++ b/src/include/rewrite/rewriteManip.h @@ -52,9 +52,8 @@ extern void AddInvertedQual(Query *parsetree, Node *qual); extern bool contain_aggs_of_level(Node *node, int levelsup); extern int locate_agg_of_level(Node *node, int levelsup); +extern bool contain_windowfuncs(Node *node); extern int locate_windowfunc(Node *node); -extern bool checkExprHasAggs(Node *node); -extern bool checkExprHasWindowFuncs(Node *node); extern bool checkExprHasSubLink(Node *node); extern Node *replace_rte_variables(Node *node, diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index 5678f066cb..6ca73a0ed7 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -292,7 +292,7 @@ select ten, sum(distinct four) from onek a group by ten having exists (select 1 from onek b where sum(distinct a.four + b.four) = b.four); -ERROR: aggregates not allowed in WHERE clause +ERROR: aggregate functions are not allowed in WHERE LINE 4: where sum(distinct a.four + b.four) = b.four)... ^ -- Test handling of sublinks within outer-level aggregates. @@ -745,6 +745,15 @@ NOTICE: drop cascades to 3 other objects DETAIL: drop cascades to table minmaxtest1 drop cascades to table minmaxtest2 drop cascades to table minmaxtest3 +-- check for correct detection of nested-aggregate errors +select max(min(unique1)) from tenk1; +ERROR: aggregate function calls cannot be nested +LINE 1: select max(min(unique1)) from tenk1; + ^ +select (select max(min(unique1)) from int8_tbl) from tenk1; +ERROR: aggregate function calls cannot be nested +LINE 1: select (select max(min(unique1)) from int8_tbl) from tenk1; + ^ -- -- Test combinations of DISTINCT and/or ORDER BY -- diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index c5b92582b4..5e17432198 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -3143,6 +3143,6 @@ LINE 1: ...m int4_tbl a full join lateral generate_series(0, a.f1) g on... DETAIL: The combining JOIN type must be INNER or LEFT for a LATERAL reference. -- LATERAL can be used to put an aggregate into the FROM clause of its query select 1 from tenk1 a, lateral (select max(a.unique1) from int4_tbl b) ss; -ERROR: aggregates not allowed in FROM clause +ERROR: aggregate functions are not allowed in FROM clause of their own query level LINE 1: select 1 from tenk1 a, lateral (select max(a.unique1) from i... ^ diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out index fde375cc9f..7778626625 100644 --- a/src/test/regress/expected/window.out +++ b/src/test/regress/expected/window.out @@ -958,32 +958,32 @@ SELECT rank() OVER (ORDER BY length('abc')); -- can't order by another window function SELECT rank() OVER (ORDER BY rank() OVER (ORDER BY random())); -ERROR: window functions not allowed in window definition +ERROR: window functions are not allowed in window definitions LINE 1: SELECT rank() OVER (ORDER BY rank() OVER (ORDER BY random())... ^ -- some other errors SELECT * FROM empsalary WHERE row_number() OVER (ORDER BY salary) < 10; -ERROR: window functions not allowed in WHERE clause +ERROR: window functions are not allowed in WHERE LINE 1: SELECT * FROM empsalary WHERE row_number() OVER (ORDER BY sa... ^ SELECT * FROM empsalary INNER JOIN tenk1 ON row_number() OVER (ORDER BY salary) < 10; -ERROR: window functions not allowed in JOIN conditions +ERROR: window functions are not allowed in JOIN conditions LINE 1: SELECT * FROM empsalary INNER JOIN tenk1 ON row_number() OVE... ^ SELECT rank() OVER (ORDER BY 1), count(*) FROM empsalary GROUP BY 1; -ERROR: window functions not allowed in GROUP BY clause +ERROR: window functions are not allowed in GROUP BY LINE 1: SELECT rank() OVER (ORDER BY 1), count(*) FROM empsalary GRO... ^ SELECT * FROM rank() OVER (ORDER BY random()); -ERROR: cannot use window function in function expression in FROM +ERROR: window functions are not allowed in functions in FROM LINE 1: SELECT * FROM rank() OVER (ORDER BY random()); ^ DELETE FROM empsalary WHERE (rank() OVER (ORDER BY random())) > 10; -ERROR: window functions not allowed in WHERE clause +ERROR: window functions are not allowed in WHERE LINE 1: DELETE FROM empsalary WHERE (rank() OVER (ORDER BY random())... ^ DELETE FROM empsalary RETURNING rank() OVER (ORDER BY random()); -ERROR: cannot use window function in RETURNING +ERROR: window functions are not allowed in RETURNING LINE 1: DELETE FROM empsalary RETURNING rank() OVER (ORDER BY random... ^ SELECT count(*) OVER w FROM tenk1 WINDOW w AS (ORDER BY unique1), w AS (ORDER BY unique1); diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out index 0d59ea3fdf..a491b2ca61 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -937,12 +937,12 @@ LINE 2: WHERE n IN (SELECT * FROM x)) -- aggregate functions WITH RECURSIVE x(n) AS (SELECT 1 UNION ALL SELECT count(*) FROM x) SELECT * FROM x; -ERROR: aggregate functions not allowed in a recursive query's recursive term +ERROR: aggregate functions are not allowed in a recursive query's recursive term LINE 1: WITH RECURSIVE x(n) AS (SELECT 1 UNION ALL SELECT count(*) F... ^ WITH RECURSIVE x(n) AS (SELECT 1 UNION ALL SELECT sum(n) FROM x) SELECT * FROM x; -ERROR: aggregate functions not allowed in a recursive query's recursive term +ERROR: aggregate functions are not allowed in a recursive query's recursive term LINE 1: WITH RECURSIVE x(n) AS (SELECT 1 UNION ALL SELECT sum(n) FRO... ^ -- ORDER BY diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index d1c74720d3..53a2183b3d 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -280,6 +280,10 @@ select min(f1), max(f1) from minmaxtest; drop table minmaxtest cascade; +-- check for correct detection of nested-aggregate errors +select max(min(unique1)) from tenk1; +select (select max(min(unique1)) from int8_tbl) from tenk1; + -- -- Test combinations of DISTINCT and/or ORDER BY --