Rearrange validity checks for plpgsql "simple" expressions.

Buildfarm experience shows what probably should've occurred to me before:
if a cache flush occurs partway through building a generic plan, then
the plansource may have is_valid = false even though the plan is valid.
We need to accept this case, use the generated plan, and then try to
replan the next time.  We can't try to replan immediately, because that
would produce an infinite loop in CLOBBER_CACHE_ALWAYS builds; moreover
it's really overkill.  (We can assume that the plan is valid, it's just
possibly a bit stale.  Note that the pre-existing code behaved this way,
and the non-simple-expression code paths do too.)  Conversely, not using
the generated plan would drop us into the not-a-simple-expression code
path, which is bad for performance and would also cause regression-test
failures due to visibly different error-reporting behavior.

Hence, refactor the validity-check functions so that the initial check
and recheck cases can react differently to plansource->is_valid.
This makes their usage a bit simpler, too.

Discussion: https://postgr.es/m/7072.1585332104@sss.pgh.pa.us

1 files changed, 13 insertions, 21 deletions

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 0663ccbd114..1c40709b3c9 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -6181,14 +6181,13 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 		Assert(cplan != NULL);
 
 		/*
-		 * These tests probably can't fail either, but if they do, cope by
+		 * This test probably can't fail either, but if it does, cope by
 		 * declaring the plan to be non-simple.  On success, we'll acquire a
 		 * refcount on the new plan, stored in simple_eval_resowner.
 		 */
 		if (CachedPlanAllowsSimpleValidityCheck(expr->expr_simple_plansource,
-												cplan) &&
-			CachedPlanIsSimplyValid(expr->expr_simple_plansource, cplan,
-									estate->simple_eval_resowner))
+												cplan,
+												estate->simple_eval_resowner))
 		{
 			/* Remember that we have the refcount */
 			expr->expr_simple_plan = cplan;
@@ -8089,26 +8088,19 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
 	/*
 	 * Verify that plancache.c thinks the plan is simple enough to use
 	 * CachedPlanIsSimplyValid.  Given the restrictions above, it's unlikely
-	 * that this could fail, but if it does, just treat plan as not simple.
+	 * that this could fail, but if it does, just treat plan as not simple. On
+	 * success, save a refcount on the plan in the simple-expression resowner.
 	 */
-	if (CachedPlanAllowsSimpleValidityCheck(plansource, cplan))
+	if (CachedPlanAllowsSimpleValidityCheck(plansource, cplan,
+											estate->simple_eval_resowner))
 	{
-		/*
-		 * OK, use CachedPlanIsSimplyValid to save a refcount on the plan in
-		 * the simple-expression resowner.  This shouldn't fail either, but if
-		 * somehow it does, again we can cope by treating plan as not simple.
-		 */
-		if (CachedPlanIsSimplyValid(plansource, cplan,
-									estate->simple_eval_resowner))
-		{
-			/* Remember that we have the refcount */
-			expr->expr_simple_plansource = plansource;
-			expr->expr_simple_plan = cplan;
-			expr->expr_simple_plan_lxid = MyProc->lxid;
+		/* Remember that we have the refcount */
+		expr->expr_simple_plansource = plansource;
+		expr->expr_simple_plan = cplan;
+		expr->expr_simple_plan_lxid = MyProc->lxid;
 
-			/* Share the remaining work with the replan code path */
-			exec_save_simple_expr(expr, cplan);
-		}
+		/* Share the remaining work with the replan code path */
+		exec_save_simple_expr(expr, cplan);
 	}
 
 	/*