From 00418c61244138bd8ac2de58076a1d0dd4f539f3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 15 Aug 2017 12:28:39 -0400 Subject: [PATCH] Simplify plpgsql's check for simple expressions. plpgsql wants to recognize expressions that it can execute directly via ExecEvalExpr() instead of going through the full SPI machinery. Originally the test for this consisted of recursively groveling through the post-planning expression tree to see if it contained only nodes that plpgsql recognized as safe. That was a major maintenance headache, since it required updating plpgsql every time we added any kind of expression node. It was also kind of expensive, so over time we added various pre-planning checks to try to short-circuit having to do that. Robert Haas pointed out that as of the SRF-processing changes in v10, particularly the addition of Query.hasTargetSRFs, there really isn't any reason to make the recursive scan at all: the initial checks cover everything we really care about. We do have to make sure that those checks agree with what inline_function() considers, so that inlining of a function that formerly wasn't inlined can't cause an expression considered simple to become non-simple. Hence, delete the recursive function exec_simple_check_node(), and tweak those other tests to more exactly agree with inline_function(). Adjust some comments and function naming to match. Discussion: https://postgr.es/m/CA+TgmoZGZpwdEV2FQWaVxA_qZXsQE1DAS5Fu8fwxXDNvfndiUQ@mail.gmail.com --- src/backend/optimizer/util/clauses.c | 5 + src/pl/plpgsql/src/pl_exec.c | 387 ++++----------------------- 2 files changed, 53 insertions(+), 339 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 8b4425dcf9..602d17dfb4 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -4445,6 +4445,11 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid, /* * The single command must be a simple "SELECT expression". + * + * Note: if you change the tests involved in this, see also plpgsql's + * exec_simple_check_plan(). That generally needs to have the same idea + * of what's a "simple expression", so that inlining a function that + * previously wasn't inlined won't change plpgsql's conclusion. */ if (!IsA(querytree, Query) || querytree->commandType != CMD_SELECT || diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index c98492b2a4..4eb2dd284b 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -224,9 +224,8 @@ static void exec_eval_cleanup(PLpgSQL_execstate *estate); static void exec_prepare_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr, int cursorOptions); -static bool exec_simple_check_node(Node *node); static void exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr); -static void exec_simple_recheck_plan(PLpgSQL_expr *expr, CachedPlan *cplan); +static void exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan); static void exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno); static bool contains_target_param(Node *node, int *target_dno); static bool exec_eval_simple_expr(PLpgSQL_execstate *estate, @@ -5488,13 +5487,12 @@ loop_exit: * of the tree was aborted by an error: the tree may contain bogus state * so we dare not re-use it. * - * It is possible though unlikely for a simple expression to become non-simple - * (consider for example redefining a trivial view). We must handle that for - * correctness; fortunately it's normally inexpensive to call - * SPI_plan_get_cached_plan for a simple expression. We do not consider the - * other direction (non-simple expression becoming simple) because we'll still - * give correct results if that happens, and it's unlikely to be worth the - * cycles to check. + * It is possible that we'd need to replan a simple expression; for example, + * someone might redefine a SQL function that had been inlined into the simple + * expression. That cannot cause a simple expression to become non-simple (or + * vice versa), but we do have to handle replacing the expression tree. + * Fortunately it's normally inexpensive to call SPI_plan_get_cached_plan for + * a simple expression. * * Note: if pass-by-reference, the result is in the eval_mcontext. * It will be freed when exec_eval_cleanup is done. @@ -5543,19 +5541,13 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, */ Assert(cplan != NULL); + /* If it got replanned, update our copy of the simple expression */ if (cplan->generation != expr->expr_simple_generation) { - /* It got replanned ... is it still simple? */ - exec_simple_recheck_plan(expr, cplan); - /* better recheck r/w safety, as well */ + exec_save_simple_expr(expr, cplan); + /* better recheck r/w safety, as it could change due to inlining */ if (expr->rwparam >= 0) exec_check_rw_parameter(expr, expr->rwparam); - if (expr->expr_simple_expr == NULL) - { - /* Oops, release refcount and fail */ - ReleaseCachedPlan(cplan, true); - return false; - } } /* @@ -5567,7 +5559,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, /* * Prepare the expression for execution, if it's not been done already in * the current transaction. (This will be forced to happen if we called - * exec_simple_recheck_plan above.) + * exec_save_simple_expr above.) */ if (expr->expr_simple_lxid != curlxid) { @@ -6450,265 +6442,6 @@ get_cast_hashentry(PLpgSQL_execstate *estate, return cast_entry; } -/* ---------- - * exec_simple_check_node - Recursively check if an expression - * is made only of simple things we can - * hand out directly to ExecEvalExpr() - * instead of calling SPI. - * ---------- - */ -static bool -exec_simple_check_node(Node *node) -{ - if (node == NULL) - return TRUE; - - switch (nodeTag(node)) - { - case T_Const: - return TRUE; - - case T_Param: - return TRUE; - - case T_ArrayRef: - { - ArrayRef *expr = (ArrayRef *) node; - - if (!exec_simple_check_node((Node *) expr->refupperindexpr)) - return FALSE; - if (!exec_simple_check_node((Node *) expr->reflowerindexpr)) - return FALSE; - if (!exec_simple_check_node((Node *) expr->refexpr)) - return FALSE; - if (!exec_simple_check_node((Node *) expr->refassgnexpr)) - return FALSE; - - return TRUE; - } - - case T_FuncExpr: - { - FuncExpr *expr = (FuncExpr *) node; - - if (expr->funcretset) - return FALSE; - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_OpExpr: - { - OpExpr *expr = (OpExpr *) node; - - if (expr->opretset) - return FALSE; - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_DistinctExpr: - { - DistinctExpr *expr = (DistinctExpr *) node; - - if (expr->opretset) - return FALSE; - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_NullIfExpr: - { - NullIfExpr *expr = (NullIfExpr *) node; - - if (expr->opretset) - return FALSE; - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_ScalarArrayOpExpr: - { - ScalarArrayOpExpr *expr = (ScalarArrayOpExpr *) node; - - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_BoolExpr: - { - BoolExpr *expr = (BoolExpr *) node; - - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_FieldSelect: - return exec_simple_check_node((Node *) ((FieldSelect *) node)->arg); - - case T_FieldStore: - { - FieldStore *expr = (FieldStore *) node; - - if (!exec_simple_check_node((Node *) expr->arg)) - return FALSE; - if (!exec_simple_check_node((Node *) expr->newvals)) - return FALSE; - - return TRUE; - } - - case T_RelabelType: - return exec_simple_check_node((Node *) ((RelabelType *) node)->arg); - - case T_CoerceViaIO: - return exec_simple_check_node((Node *) ((CoerceViaIO *) node)->arg); - - case T_ArrayCoerceExpr: - return exec_simple_check_node((Node *) ((ArrayCoerceExpr *) node)->arg); - - case T_ConvertRowtypeExpr: - return exec_simple_check_node((Node *) ((ConvertRowtypeExpr *) node)->arg); - - case T_CaseExpr: - { - CaseExpr *expr = (CaseExpr *) node; - - if (!exec_simple_check_node((Node *) expr->arg)) - return FALSE; - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - if (!exec_simple_check_node((Node *) expr->defresult)) - return FALSE; - - return TRUE; - } - - case T_CaseWhen: - { - CaseWhen *when = (CaseWhen *) node; - - if (!exec_simple_check_node((Node *) when->expr)) - return FALSE; - if (!exec_simple_check_node((Node *) when->result)) - return FALSE; - - return TRUE; - } - - case T_CaseTestExpr: - return TRUE; - - case T_ArrayExpr: - { - ArrayExpr *expr = (ArrayExpr *) node; - - if (!exec_simple_check_node((Node *) expr->elements)) - return FALSE; - - return TRUE; - } - - case T_RowExpr: - { - RowExpr *expr = (RowExpr *) node; - - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_RowCompareExpr: - { - RowCompareExpr *expr = (RowCompareExpr *) node; - - if (!exec_simple_check_node((Node *) expr->largs)) - return FALSE; - if (!exec_simple_check_node((Node *) expr->rargs)) - return FALSE; - - return TRUE; - } - - case T_CoalesceExpr: - { - CoalesceExpr *expr = (CoalesceExpr *) node; - - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_MinMaxExpr: - { - MinMaxExpr *expr = (MinMaxExpr *) node; - - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_SQLValueFunction: - return TRUE; - - case T_XmlExpr: - { - XmlExpr *expr = (XmlExpr *) node; - - if (!exec_simple_check_node((Node *) expr->named_args)) - return FALSE; - if (!exec_simple_check_node((Node *) expr->args)) - return FALSE; - - return TRUE; - } - - case T_NullTest: - return exec_simple_check_node((Node *) ((NullTest *) node)->arg); - - case T_BooleanTest: - return exec_simple_check_node((Node *) ((BooleanTest *) node)->arg); - - case T_CoerceToDomain: - return exec_simple_check_node((Node *) ((CoerceToDomain *) node)->arg); - - case T_CoerceToDomainValue: - return TRUE; - - case T_List: - { - List *expr = (List *) node; - ListCell *l; - - foreach(l, expr) - { - if (!exec_simple_check_node(lfirst(l))) - return FALSE; - } - - return TRUE; - } - - default: - return FALSE; - } -} - /* ---------- * exec_simple_check_plan - Check if a plan is simple enough to @@ -6726,12 +6459,16 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) MemoryContext oldcontext; /* - * Initialize to "not simple", and remember the plan generation number we - * last checked. (If we don't get as far as obtaining a plan to check, we - * just leave expr_simple_generation set to 0.) + * Initialize to "not simple". */ expr->expr_simple_expr = NULL; - expr->expr_simple_generation = 0; + + /* + * Check the analyzed-and-rewritten form of the query to see if we will be + * able to treat it as a simple expression. Since this function is only + * called immediately after creating the CachedPlanSource, we need not + * worry about the query being stale. + */ /* * We can only test queries that resulted in exactly one CachedPlanSource @@ -6741,15 +6478,6 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) return; plansource = (CachedPlanSource *) linitial(plansources); - /* - * Do some checking on the analyzed-and-rewritten form of the query. These - * checks are basically redundant with the tests in - * exec_simple_recheck_plan, but the point is to avoid building a plan if - * possible. Since this function is only called immediately after - * creating the CachedPlanSource, we need not worry about the query being - * stale. - */ - /* * 1. There must be one single querytree. */ @@ -6768,16 +6496,20 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) return; /* - * 3. Can't have any subplans, aggregates, qual clauses either + * 3. Can't have any subplans, aggregates, qual clauses either. (These + * tests should generally match what inline_function() checks before + * inlining a SQL function; otherwise, inlining could change our + * conclusion about whether an expression is simple, which we don't want.) */ if (query->hasAggs || query->hasWindowFuncs || query->hasTargetSRFs || query->hasSubLinks || - query->hasForUpdate || query->cteList || + query->jointree->fromlist || query->jointree->quals || query->groupClause || + query->groupingSets || query->havingQual || query->windowClause || query->distinctClause || @@ -6794,7 +6526,7 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) return; /* - * OK, it seems worth constructing a plan for more careful checking. + * OK, we can treat it as a simple plan. * * Get the generic plan for the query. If replanning is needed, do that * work in the eval_mcontext. @@ -6806,75 +6538,52 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) /* Can't fail, because we checked for a single CachedPlanSource above */ Assert(cplan != NULL); - /* Share the remaining work with recheck code path */ - exec_simple_recheck_plan(expr, cplan); + /* Share the remaining work with replan code path */ + exec_save_simple_expr(expr, cplan); /* Release our plan refcount */ ReleaseCachedPlan(cplan, true); } /* - * exec_simple_recheck_plan --- check for simple plan once we have CachedPlan + * exec_save_simple_expr --- extract simple expression from CachedPlan */ static void -exec_simple_recheck_plan(PLpgSQL_expr *expr, CachedPlan *cplan) +exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan) { PlannedStmt *stmt; Plan *plan; TargetEntry *tle; /* - * Initialize to "not simple", and remember the plan generation number we - * last checked. + * Given the checks that exec_simple_check_plan did, none of the Asserts + * here should ever fail. */ - expr->expr_simple_expr = NULL; - expr->expr_simple_generation = cplan->generation; - /* - * 1. There must be one single plantree - */ - if (list_length(cplan->stmt_list) != 1) - return; + /* Extract the single PlannedStmt */ + Assert(list_length(cplan->stmt_list) == 1); stmt = linitial_node(PlannedStmt, cplan->stmt_list); - /* - * 2. It must be a RESULT plan --> no scan's required - */ - if (stmt->commandType != CMD_SELECT) - return; + /* Should be a trivial Result plan */ + Assert(stmt->commandType == CMD_SELECT); plan = stmt->planTree; - if (!IsA(plan, Result)) - return; - - /* - * 3. Can't have any subplan or qual clause, either - */ - if (plan->lefttree != NULL || - plan->righttree != NULL || - plan->initPlan != NULL || - plan->qual != NULL || - ((Result *) plan)->resconstantqual != NULL) - return; - - /* - * 4. The plan must have a single attribute as result - */ - if (list_length(plan->targetlist) != 1) - return; - + Assert(IsA(plan, Result)); + Assert(plan->lefttree == NULL && + plan->righttree == NULL && + plan->initPlan == NULL && + plan->qual == NULL && + ((Result *) plan)->resconstantqual == NULL); + + /* Extract the single tlist expression */ + Assert(list_length(plan->targetlist) == 1); tle = (TargetEntry *) linitial(plan->targetlist); /* - * 5. Check that all the nodes in the expression are non-scary. - */ - if (!exec_simple_check_node((Node *) tle->expr)) - return; - - /* - * Yes - this is a simple expression. Mark it as such, and initialize - * state to "not valid in current transaction". + * Save the simple expression, and initialize state to "not valid in + * current transaction". */ expr->expr_simple_expr = tle->expr; + expr->expr_simple_generation = cplan->generation; expr->expr_simple_state = NULL; expr->expr_simple_in_use = false; expr->expr_simple_lxid = InvalidLocalTransactionId; -- 2.40.0