Re: [HACKERS] Is exec_simple_check_node still doing anything?

[Tom Lane]

I wrote:
> Robert Haas  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
! 	 * 

Re: [HACKERS] Is exec_simple_check_node still doing anything?

[Tom Lane]

Robert Haas  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.

I think you might be right.  The other way that I'm aware of that
could cause interesting things to happen is if someone redefines
a SQL function that had been inlined in the originally-compiled
version of the expression.  However, it should be the case that
inline_function() will refuse to inline if the new definition
contains anything "scary", so that the expression as seen by
plpgsql is still simple; any non-simplicity will just be hidden
under a function call.

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".

If I recall things correctly, originally there were only the
post-planning simplicity checks that are now embodied in
exec_simple_recheck_plan/exec_simple_check_node.  I think I added
on the pre-planning checks in exec_simple_check_plan in order to
try to save some planning cycles.  Since the SRF checks were
clearly still necessary at the time, I didn't think hard about
whether any of the other post-planning checks could be got rid of.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Is exec_simple_check_node still doing anything?

[Robert Haas]

I'm a little mystified by exec_simple_check_node().  The regression
tests seem not to exercise it.  It can only be reached when
exec_simple_recheck_plan() finds no other reason to reject the plan,
and the only case it seems to reject is the one where there's a
set-returning function buried in there someplace.  But then it seems
like hasTargetSRFs would have been true and we would have given up
before making a plan in the first place.  Of course, that only
protects us when originally forming the plan; they don't account for
later changes -- and the code comments claim that an expression which
was originally simple can become non-simple:

 * It is possible though unlikely for a simple expression to become non-simple
 * (consider for example redefining a trivial view).

But I can't quite figure that one out.  If we're selecting from a
trivial view, then the range table won't be empty and the expression
won't be simple in the first place.  The check for a non-empty range
table didn't exist when this comment was originally added
(95f6d2d20921b7c2dbec29bf2706fd9448208aa6, 2007); it was added in a
subsequent redesign (e6faf910d75027bdce7cd0f2033db4e912592bcc; 2011).
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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers