Re: [HACKERS] Is exec_simple_check_node still doing anything?

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