Skip to content

Commit

Permalink
Centralize the logic for detecting misplaced aggregates, window funcs…
Browse files Browse the repository at this point in the history
…, 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.)
  • Loading branch information
tglsfdc committed Aug 10, 2012
1 parent b3055ab commit eaccfde
Show file tree
Hide file tree
Showing 29 changed files with 952 additions and 781 deletions.
45 changes: 9 additions & 36 deletions src/backend/catalog/heap.c
Original file line number Diff line number Diff line change
Expand Up @@ -2415,40 +2415,28 @@ 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,
(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
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))
ereport(ERROR,
(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 ---
Expand Down Expand Up @@ -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.
Expand All @@ -2512,30 +2500,15 @@ 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,
(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
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;
}

Expand Down
24 changes: 8 additions & 16 deletions src/backend/commands/functioncmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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;
Expand Down
26 changes: 5 additions & 21 deletions src/backend/commands/indexcmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
16 changes: 1 addition & 15 deletions src/backend/commands/prepare.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
17 changes: 2 additions & 15 deletions src/backend/commands/tablecmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
17 changes: 1 addition & 16 deletions src/backend/commands/trigger.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
33 changes: 5 additions & 28 deletions src/backend/commands/typecmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
*/
Expand Down
2 changes: 1 addition & 1 deletion src/backend/optimizer/util/clauses.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/*
Expand Down
Loading

0 comments on commit eaccfde

Please sign in to comment.