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

Author: tglsfdc

src/backend/utils/cache/plancache.c

@@ -1290,6 +1290,10 @@ ReleaseCachedPlan(CachedPlan *plan, bool useResOwner)
  * to be invalidated, for example due to a change in a function that was
  * inlined into the plan.)
  *
+ * If the plan is simply valid, and "owner" is not NULL, record a refcount on
+ * the plan in that resowner before returning.  It is caller's responsibility
+ * to be sure that a refcount is held on any plan that's being actively used.
+ *
  * This must only be called on known-valid generic plans (eg, ones just
  * returned by GetCachedPlan).  If it returns true, the caller may re-use
  * the cached plan as long as CachedPlanIsSimplyValid returns true; that
@@ -1298,16 +1302,24 @@ ReleaseCachedPlan(CachedPlan *plan, bool useResOwner)
  */
 bool
 CachedPlanAllowsSimpleValidityCheck(CachedPlanSource *plansource,
-									CachedPlan *plan)
+									CachedPlan *plan, ResourceOwner owner)
 {
 	ListCell   *lc;
 
-	/* Sanity-check that the caller gave us a validated generic plan. */
+	/*
+	 * Sanity-check that the caller gave us a validated generic plan.  Notice
+	 * that we *don't* assert plansource->is_valid as you might expect; that's
+	 * because it's possible that that's already false when GetCachedPlan
+	 * returns, e.g. because ResetPlanCache happened partway through.  We
+	 * should accept the plan as long as plan->is_valid is true, and expect to
+	 * replan after the next CachedPlanIsSimplyValid call.
+	 */
 	Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);
 	Assert(plan->magic == CACHEDPLAN_MAGIC);
-	Assert(plansource->is_valid);
 	Assert(plan->is_valid);
 	Assert(plan == plansource->gplan);
+	Assert(plansource->search_path != NULL);
+	Assert(OverrideSearchPathMatchesCurrent(plansource->search_path));
 
 	/* We don't support oneshot plans here. */
 	if (plansource->is_oneshot)
@@ -1371,6 +1383,15 @@ CachedPlanAllowsSimpleValidityCheck(CachedPlanSource *plansource,
 	 * Okay, it's simple.  Note that what we've primarily established here is
 	 * that no locks need be taken before checking the plan's is_valid flag.
 	 */
+
+	/* Bump refcount if requested. */
+	if (owner)
+	{
+		ResourceOwnerEnlargePlanCacheRefs(owner);
+		plan->refcount++;
+		ResourceOwnerRememberPlanCacheRef(owner, plan);
+	}
+
 	return true;
 }
 
@@ -1408,7 +1429,9 @@ CachedPlanIsSimplyValid(CachedPlanSource *plansource, CachedPlan *plan,
 
 	/*
 	 * Has cache invalidation fired on this plan?  We can check this right
-	 * away since there are no locks that we'd need to acquire first.
+	 * away since there are no locks that we'd need to acquire first.  Note
+	 * that here we *do* check plansource->is_valid, so as to force plan
+	 * rebuild if that's become false.
 	 */
 	if (!plansource->is_valid || plan != plansource->gplan || !plan->is_valid)
 		return false;

src/include/utils/plancache.h

@@ -223,7 +223,8 @@ extern CachedPlan *GetCachedPlan(CachedPlanSource *plansource,
 extern void ReleaseCachedPlan(CachedPlan *plan, bool useResOwner);
 
 extern bool CachedPlanAllowsSimpleValidityCheck(CachedPlanSource *plansource,
-												CachedPlan *plan);
+												CachedPlan *plan,
+												ResourceOwner owner);
 extern bool CachedPlanIsSimplyValid(CachedPlanSource *plansource,
 									CachedPlan *plan,
 									ResourceOwner owner);

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);
 	}
 
 	/*