Re: [HACKERS] Is exec_simple_check_node still doing anything? - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: [HACKERS] Is exec_simple_check_node still doing anything? |
Date | |
Msg-id | 2257.1498412915@sss.pgh.pa.us Whole thread Raw |
In response to | Re: [HACKERS] Is exec_simple_check_node still doing anything? (Tom Lane <tgl@sss.pgh.pa.us>) |
List | pgsql-hackers |
I wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I'm a little mystified by exec_simple_check_node(). >> ... >> Did that, possibly, remove the last way in which a simple expression >> could be could become non-simple? If so, between that and the new >> hasTargetSRFs test, it might now be impossible for >> exec_simple_check_node() to fail. > In fact, I suspect we could get rid of exec_simple_recheck_plan > altogether. It could use a bit more study, but the empty-rtable > check plus the other checks in exec_simple_check_plan (particularly, > hasAggs, hasWindowFuncs, hasTargetSRFs, hasSubLinks) seem like > they are enough to guarantee that what comes out of the planner > will be "simple". I did some more studying and it definitely seems like exec_simple_recheck_plan can never fail anymore. As an experimental check, converting everything it was testing into Asserts still gets through check-world. Accordingly, here's a proposed patch that gets rid of exec_simple_check_node() and simplifies some of the rest of the logic. regards, tom lane diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index c98492b..4eb2dd2 100644 *** a/src/pl/plpgsql/src/pl_exec.c --- b/src/pl/plpgsql/src/pl_exec.c *************** static void exec_eval_cleanup(PLpgSQL_ex *** 224,232 **** 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_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, --- 224,231 ---- static void exec_prepare_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr, int cursorOptions); static void exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr); ! 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, *************** loop_exit: *** 5488,5500 **** * 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. * * Note: if pass-by-reference, the result is in the eval_mcontext. * It will be freed when exec_eval_cleanup is done. --- 5487,5498 ---- * of the tree was aborted by an error: the tree may contain bogus state * so we dare not re-use it. * ! * 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. *************** exec_eval_simple_expr(PLpgSQL_execstate *** 5543,5561 **** */ Assert(cplan != NULL); 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 */ 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; - } } /* --- 5541,5553 ---- */ Assert(cplan != NULL); + /* If it got replanned, update our copy of the simple expression */ if (cplan->generation != expr->expr_simple_generation) { ! 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); } /* *************** exec_eval_simple_expr(PLpgSQL_execstate *** 5567,5573 **** /* * 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.) */ if (expr->expr_simple_lxid != curlxid) { --- 5559,5565 ---- /* * 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_save_simple_expr above.) */ if (expr->expr_simple_lxid != curlxid) { *************** get_cast_hashentry(PLpgSQL_execstate *es *** 6450,6714 **** 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 --- 6442,6447 ---- *************** exec_simple_check_plan(PLpgSQL_execstate *** 6726,6737 **** 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.) */ expr->expr_simple_expr = NULL; ! expr->expr_simple_generation = 0; /* * We can only test queries that resulted in exactly one CachedPlanSource --- 6459,6474 ---- MemoryContext oldcontext; /* ! * Initialize to "not simple". */ expr->expr_simple_expr = NULL; ! ! /* ! * 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 *************** exec_simple_check_plan(PLpgSQL_execstate *** 6742,6756 **** 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. */ if (list_length(plansource->query_list) != 1) --- 6479,6484 ---- *************** exec_simple_check_plan(PLpgSQL_execstate *** 6768,6783 **** return; /* ! * 3. Can't have any subplans, aggregates, qual clauses either */ if (query->hasAggs || query->hasWindowFuncs || query->hasTargetSRFs || query->hasSubLinks || - query->hasForUpdate || query->cteList || query->jointree->quals || query->groupClause || query->havingQual || query->windowClause || query->distinctClause || --- 6496,6515 ---- return; /* ! * 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->cteList || + query->jointree->fromlist || query->jointree->quals || query->groupClause || + query->groupingSets || query->havingQual || query->windowClause || query->distinctClause || *************** exec_simple_check_plan(PLpgSQL_execstate *** 6794,6800 **** return; /* ! * OK, it seems worth constructing a plan for more careful checking. * * Get the generic plan for the query. If replanning is needed, do that * work in the eval_mcontext. --- 6526,6532 ---- return; /* ! * 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. *************** exec_simple_check_plan(PLpgSQL_execstate *** 6806,6880 **** /* 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); /* Release our plan refcount */ ReleaseCachedPlan(cplan, true); } /* ! * exec_simple_recheck_plan --- check for simple plan once we have CachedPlan */ static void ! exec_simple_recheck_plan(PLpgSQL_expr *expr, CachedPlan *cplan) { PlannedStmt *stmt; Plan *plan; TargetEntry *tle; /* ! * Initialize to "not simple", and remember the plan generation number we ! * last checked. */ - 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; stmt = linitial_node(PlannedStmt, cplan->stmt_list); ! /* ! * 2. It must be a RESULT plan --> no scan's required ! */ ! if (stmt->commandType != CMD_SELECT) ! return; 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; 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". */ expr->expr_simple_expr = tle->expr; expr->expr_simple_state = NULL; expr->expr_simple_in_use = false; expr->expr_simple_lxid = InvalidLocalTransactionId; --- 6538,6589 ---- /* Can't fail, because we checked for a single CachedPlanSource above */ Assert(cplan != NULL); ! /* Share the remaining work with replan code path */ ! exec_save_simple_expr(expr, cplan); /* Release our plan refcount */ ReleaseCachedPlan(cplan, true); } /* ! * exec_save_simple_expr --- extract simple expression from CachedPlan */ static void ! exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan) { PlannedStmt *stmt; Plan *plan; TargetEntry *tle; /* ! * Given the checks that exec_simple_check_plan did, none of the Asserts ! * here should ever fail. */ ! /* Extract the single PlannedStmt */ ! Assert(list_length(cplan->stmt_list) == 1); stmt = linitial_node(PlannedStmt, cplan->stmt_list); ! /* Should be a trivial Result plan */ ! Assert(stmt->commandType == CMD_SELECT); plan = stmt->planTree; ! 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); /* ! * 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; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
pgsql-hackers by date: