Change plan caching to honor, not resist, changes in search_path.

In the initial implementation of plan caching, we saved the active
search_path when a plan was first cached, then reinstalled that path
anytime we needed to reparse or replan.  The idea of that was to try to
reselect the same referenced objects, in somewhat the same way that views
continue to refer to the same objects in the face of schema or name
changes.  Of course, that analogy doesn't bear close inspection, since
holding the search_path fixed doesn't cope with object drops or renames.
Moreover sticking with the old path seems to create more surprises than
it avoids.  So instead of doing that, consider that the cached plan depends
on search_path, and force reparse/replan if the active search_path is
different than it was when we last saved the plan.

This gets us fairly close to having "transparency" of plan caching, in the
sense that the cached statement acts the same as if you'd just resubmitted
the original query text for another execution.  There are still some corner
cases where this fails though: a new object added in the search path
schema(s) might capture a reference in the query text, but we'd not realize
that and force a reparse.  We might try to fix that in the future, but for
the moment it looks too expensive and complicated.

1 files changed, 43 insertions, 41 deletions

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index cbc7c498d0d..4630c44e740 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -15,13 +15,15 @@
  * that matches the event is marked invalid, as is its generic CachedPlan
  * if it has one.  When (and if) the next demand for a cached plan occurs,
  * parse analysis and rewrite is repeated to build a new valid query tree,
- * and then planning is performed as normal.
+ * and then planning is performed as normal.  We also force re-analysis and
+ * re-planning if the active search_path is different from the previous time.
  *
  * Note that if the sinval was a result of user DDL actions, parse analysis
  * could throw an error, for example if a column referenced by the query is
- * no longer present.  The creator of a cached plan can specify whether it
- * is allowable for the query to change output tupdesc on replan (this
- * could happen with "SELECT *" for example) --- if so, it's up to the
+ * no longer present.  Another possibility is for the query's output tupdesc
+ * to change (for instance "SELECT *" might expand differently than before).
+ * The creator of a cached plan can specify whether it is allowable for the
+ * query to change output tupdesc on replan --- if so, it's up to the
  * caller to notice changes and cope with them.
  *
  * Currently, we track exactly the dependencies of plans on relations and
@@ -174,11 +176,11 @@ CreateCachedPlan(Node *raw_parse_tree,
 	plansource->cursor_options = 0;
 	plansource->fixed_result = false;
 	plansource->resultDesc = NULL;
-	plansource->search_path = NULL;
 	plansource->context = source_context;
 	plansource->query_list = NIL;
 	plansource->relationOids = NIL;
 	plansource->invalItems = NIL;
+	plansource->search_path = NULL;
 	plansource->query_context = NULL;
 	plansource->gplan = NULL;
 	plansource->is_oneshot = false;
@@ -239,11 +241,11 @@ CreateOneShotCachedPlan(Node *raw_parse_tree,
 	plansource->cursor_options = 0;
 	plansource->fixed_result = false;
 	plansource->resultDesc = NULL;
-	plansource->search_path = NULL;
 	plansource->context = CurrentMemoryContext;
 	plansource->query_list = NIL;
 	plansource->relationOids = NIL;
 	plansource->invalItems = NIL;
+	plansource->search_path = NULL;
 	plansource->query_context = NULL;
 	plansource->gplan = NULL;
 	plansource->is_oneshot = true;
@@ -361,6 +363,14 @@ CompleteCachedPlan(CachedPlanSource *plansource,
 								   &plansource->invalItems);
 
 	/*
+	 * Also save the current search_path in the query_context.  (This should
+	 * not generate much extra cruft either, since almost certainly the path
+	 * is already valid.)  Again, don't really need it for one-shot plans.
+	 */
+	if (!plansource->is_oneshot)
+		plansource->search_path = GetOverrideSearchPath(querytree_context);
+
+	/*
 	 * Save the final parameter types (or other parameter specification data)
 	 * into the source_context, as well as our other parameters.  Also save
 	 * the result tuple descriptor.
@@ -383,12 +393,6 @@ CompleteCachedPlan(CachedPlanSource *plansource,
 
 	MemoryContextSwitchTo(oldcxt);
 
-	/*
-	 * Fetch current search_path into dedicated context, but do any
-	 * recalculation work required in caller's context.
-	 */
-	plansource->search_path = GetOverrideSearchPath(source_context);
-
 	plansource->is_complete = true;
 	plansource->is_valid = true;
 }
@@ -547,6 +551,23 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
 	}
 
 	/*
+	 * If the query is currently valid, we should have a saved search_path ---
+	 * check to see if that matches the current environment.  If not, we want
+	 * to force replan.
+	 */
+	if (plansource->is_valid)
+	{
+		Assert(plansource->search_path != NULL);
+		if (!OverrideSearchPathMatchesCurrent(plansource->search_path))
+		{
+			/* Invalidate the querytree and generic plan */
+			plansource->is_valid = false;
+			if (plansource->gplan)
+				plansource->gplan->is_valid = false;
+		}
+	}
+
+	/*
 	 * If the query is currently valid, acquire locks on the referenced
 	 * objects; then check again.  We need to do it this way to cover the race
 	 * condition that an invalidation message arrives before we get the locks.
@@ -578,6 +599,7 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
 	plansource->query_list = NIL;
 	plansource->relationOids = NIL;
 	plansource->invalItems = NIL;
+	plansource->search_path = NULL;
 
 	/*
 	 * Free the query_context.	We don't really expect MemoryContextDelete to
@@ -603,14 +625,6 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
 	Assert(plansource->is_complete);
 
 	/*
-	 * Restore the search_path that was in use when the plan was made. See
-	 * comments for PushOverrideSearchPath about limitations of this.
-	 *
-	 * (XXX is there anything else we really need to restore?)
-	 */
-	PushOverrideSearchPath(plansource->search_path);
-
-	/*
 	 * If a snapshot is already set (the normal case), we can just use that
 	 * for parsing/planning.  But if it isn't, install one.  Note: no point in
 	 * checking whether parse analysis requires a snapshot; utility commands
@@ -645,9 +659,6 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
 	if (snapshot_set)
 		PopActiveSnapshot();
 
-	/* Now we can restore current search path */
-	PopOverrideSearchPath();
-
 	/*
 	 * Check or update the result tupdesc.	XXX should we use a weaker
 	 * condition than equalTupleDescs() here?
@@ -699,6 +710,13 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
 							   &plansource->relationOids,
 							   &plansource->invalItems);
 
+	/*
+	 * Also save the current search_path in the query_context.  (This should
+	 * not generate much extra cruft either, since almost certainly the path
+	 * is already valid.)
+	 */
+	plansource->search_path = GetOverrideSearchPath(querytree_context);
+
 	MemoryContextSwitchTo(oldcxt);
 
 	/* Now reparent the finished query_context and save the links */
@@ -849,20 +867,6 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
 	}
 
 	/*
-	 * Restore the search_path that was in use when the plan was made. See
-	 * comments for PushOverrideSearchPath about limitations of this.
-	 *
-	 * (XXX is there anything else we really need to restore?)
-	 *
-	 * Note: it's a bit annoying to do this and snapshot-setting twice in the
-	 * case where we have to do both re-analysis and re-planning.  However,
-	 * until there's some evidence that the cost is actually meaningful
-	 * compared to parse analysis + planning, I'm not going to contort the
-	 * code enough to avoid that.
-	 */
-	PushOverrideSearchPath(plansource->search_path);
-
-	/*
 	 * If a snapshot is already set (the normal case), we can just use that
 	 * for planning.  But if it isn't, and we need one, install one.
 	 */
@@ -894,9 +898,6 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
 	if (snapshot_set)
 		PopActiveSnapshot();
 
-	/* Now we can restore current search path */
-	PopOverrideSearchPath();
-
 	/*
 	 * Normally we make a dedicated memory context for the CachedPlan and its
 	 * subsidiary data.  (It's probably not going to be large, but just in
@@ -1268,7 +1269,6 @@ CopyCachedPlan(CachedPlanSource *plansource)
 		newsource->resultDesc = CreateTupleDescCopy(plansource->resultDesc);
 	else
 		newsource->resultDesc = NULL;
-	newsource->search_path = CopyOverrideSearchPath(plansource->search_path);
 	newsource->context = source_context;
 
 	querytree_context = AllocSetContextCreate(source_context,
@@ -1280,6 +1280,8 @@ CopyCachedPlan(CachedPlanSource *plansource)
 	newsource->query_list = (List *) copyObject(plansource->query_list);
 	newsource->relationOids = (List *) copyObject(plansource->relationOids);
 	newsource->invalItems = (List *) copyObject(plansource->invalItems);
+	if (plansource->search_path)
+		newsource->search_path = CopyOverrideSearchPath(plansource->search_path);
 	newsource->query_context = querytree_context;
 
 	newsource->gplan = NULL;