I wrote:
> Robert Haas <robertmh...@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
