Avoid invalidating all foreign-join cached plans when user mappings change.

We must not push down a foreign join when the foreign tables involved
should be accessed under different user mappings.  Previously we tried
to enforce that rule literally during planning, but that meant that the
resulting plans were dependent on the current contents of the
pg_user_mapping catalog, and we had to blow away all cached plans
containing any remote join when anything at all changed in pg_user_mapping.
This could have been improved somewhat, but the fact that a syscache inval
callback has very limited info about what changed made it hard to do better
within that design.  Instead, let's change the planner to not consider user
mappings per se, but to allow a foreign join if both RTEs have the same
checkAsUser value.  If they do, then they necessarily will use the same
user mapping at runtime, and we don't need to know specifically which one
that is.  Post-plan-time changes in pg_user_mapping no longer require any
plan invalidation.

This rule does give up some optimization ability, to wit where two foreign
table references come from views with different owners or one's from a view
and one's directly in the query, but nonetheless the same user mapping
would have applied.  We'll sacrifice the first case, but to not regress
more than we have to in the second case, allow a foreign join involving
both zero and nonzero checkAsUser values if the nonzero one is the same as
the prevailing effective userID.  In that case, mark the plan as only
runnable by that userID.

The plancache code already had a notion of plans being userID-specific,
in order to support RLS.  It was a little confused though, in particular
lacking clarity of thought as to whether it was the rewritten query or just
the finished plan that's dependent on the userID.  Rearrange that code so
that it's clearer what depends on which, and so that the same logic applies
to both RLS-injected role dependency and foreign-join-injected role
dependency.

Note that this patch doesn't remove the other issue mentioned in the
original complaint, which is that while we'll reliably stop using a foreign
join if it's disallowed in a new context, we might fail to start using a
foreign join if it's now allowed, but we previously created a generic
cached plan that didn't use one.  It was agreed that the chance of winning
that way was not high enough to justify the much larger number of plan
invalidations that would have to occur if we tried to cause it to happen.

In passing, clean up randomly-varying spelling of EXPLAIN commands in
postgres_fdw.sql, and fix a COSTS ON example that had been allowed to
leak into the committed tests.

This reverts most of commits fbe5a3fb7 and 5d4171d1c, which were the
previous attempt at ensuring we wouldn't push down foreign joins that
span permissions contexts.

Etsuro Fujita and Tom Lane

Discussion: <d49c1e5b-f059-20f4-c132-e9752ee0113e@lab.ntt.co.jp>

1 files changed, 55 insertions, 123 deletions

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 005e4b7f1c3..f42a62d5000 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -100,13 +100,10 @@ static void AcquireExecutorLocks(List *stmt_list, bool acquire);
 static void AcquirePlannerLocks(List *stmt_list, bool acquire);
 static void ScanQueryForLocks(Query *parsetree, bool acquire);
 static bool ScanQueryWalker(Node *node, bool *acquire);
-static bool plan_list_is_transient(List *stmt_list);
 static TupleDesc PlanCacheComputeResultDesc(List *stmt_list);
 static void PlanCacheRelCallback(Datum arg, Oid relid);
 static void PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue);
 static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
-static void PlanCacheUserMappingCallback(Datum arg, int cacheid,
-							 uint32 hashvalue);
 
 
 /*
@@ -122,8 +119,6 @@ InitPlanCache(void)
 	CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
 	CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
 	CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
-	/* User mapping change may invalidate plans with pushed down foreign join */
-	CacheRegisterSyscacheCallback(USERMAPPINGOID, PlanCacheUserMappingCallback, (Datum) 0);
 }
 
 /*
@@ -198,6 +193,9 @@ CreateCachedPlan(Node *raw_parse_tree,
 	plansource->invalItems = NIL;
 	plansource->search_path = NULL;
 	plansource->query_context = NULL;
+	plansource->rewriteRoleId = InvalidOid;
+	plansource->rewriteRowSecurity = false;
+	plansource->dependsOnRLS = false;
 	plansource->gplan = NULL;
 	plansource->is_oneshot = false;
 	plansource->is_complete = false;
@@ -208,9 +206,6 @@ CreateCachedPlan(Node *raw_parse_tree,
 	plansource->generic_cost = -1;
 	plansource->total_custom_cost = 0;
 	plansource->num_custom_plans = 0;
-	plansource->hasRowSecurity = false;
-	plansource->planUserId = InvalidOid;
-	plansource->row_security_env = false;
 
 	MemoryContextSwitchTo(oldcxt);
 
@@ -266,6 +261,9 @@ CreateOneShotCachedPlan(Node *raw_parse_tree,
 	plansource->invalItems = NIL;
 	plansource->search_path = NULL;
 	plansource->query_context = NULL;
+	plansource->rewriteRoleId = InvalidOid;
+	plansource->rewriteRowSecurity = false;
+	plansource->dependsOnRLS = false;
 	plansource->gplan = NULL;
 	plansource->is_oneshot = true;
 	plansource->is_complete = false;
@@ -276,8 +274,6 @@ CreateOneShotCachedPlan(Node *raw_parse_tree,
 	plansource->generic_cost = -1;
 	plansource->total_custom_cost = 0;
 	plansource->num_custom_plans = 0;
-	plansource->planUserId = InvalidOid;
-	plansource->row_security_env = false;
 
 	return plansource;
 }
@@ -384,7 +380,11 @@ CompleteCachedPlan(CachedPlanSource *plansource,
 		extract_query_dependencies((Node *) querytree_list,
 								   &plansource->relationOids,
 								   &plansource->invalItems,
-								   &plansource->hasRowSecurity);
+								   &plansource->dependsOnRLS);
+
+		/* Update RLS info as well. */
+		plansource->rewriteRoleId = GetUserId();
+		plansource->rewriteRowSecurity = row_security;
 
 		/*
 		 * Also save the current search_path in the query_context.  (This
@@ -416,8 +416,6 @@ CompleteCachedPlan(CachedPlanSource *plansource,
 	plansource->cursor_options = cursor_options;
 	plansource->fixed_result = fixed_result;
 	plansource->resultDesc = PlanCacheComputeResultDesc(querytree_list);
-	plansource->planUserId = GetUserId();
-	plansource->row_security_env = row_security;
 
 	MemoryContextSwitchTo(oldcxt);
 
@@ -583,12 +581,10 @@ 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.  We should also have a valid planUserId.
+	 * to force replan.
 	 */
 	if (plansource->is_valid)
 	{
-		Assert(OidIsValid(plansource->planUserId));
-
 		Assert(plansource->search_path != NULL);
 		if (!OverrideSearchPathMatchesCurrent(plansource->search_path))
 		{
@@ -600,29 +596,15 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
 	}
 
 	/*
-	 * If the plan has a possible RLS dependency, force a replan if either the
-	 * role or the row_security setting has changed.
+	 * If the query rewrite phase had a possible RLS dependency, we must redo
+	 * it if either the role or the row_security setting has changed.
 	 */
-	if (plansource->is_valid
-		&& plansource->hasRowSecurity
-		&& (plansource->planUserId != GetUserId()
-			|| plansource->row_security_env != row_security))
+	if (plansource->is_valid && plansource->dependsOnRLS &&
+		(plansource->rewriteRoleId != GetUserId() ||
+		 plansource->rewriteRowSecurity != row_security))
 		plansource->is_valid = false;
 
 	/*
-	 * If we have a join pushed down to the foreign server and the current
-	 * user is different from the one for which the plan was created,
-	 * invalidate the generic plan since user mapping for the new user might
-	 * make the join unsafe to push down, or change which user mapping is
-	 * used.
-	 */
-	if (plansource->is_valid &&
-		plansource->gplan &&
-		plansource->gplan->has_foreign_join &&
-		plansource->planUserId != GetUserId())
-		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.
@@ -657,14 +639,6 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
 	plansource->search_path = NULL;
 
 	/*
-	 * The plan is invalid, possibly due to row security, so we need to reset
-	 * row_security_env and planUserId as we're about to re-plan with the
-	 * current settings.
-	 */
-	plansource->row_security_env = row_security;
-	plansource->planUserId = GetUserId();
-
-	/*
 	 * Free the query_context.  We don't really expect MemoryContextDelete to
 	 * fail, but just in case, make sure the CachedPlanSource is left in a
 	 * reasonably sane state.  (The generic plan won't get unlinked yet, but
@@ -774,7 +748,11 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
 	extract_query_dependencies((Node *) qlist,
 							   &plansource->relationOids,
 							   &plansource->invalItems,
-							   &plansource->hasRowSecurity);
+							   &plansource->dependsOnRLS);
+
+	/* Update RLS info as well. */
+	plansource->rewriteRoleId = GetUserId();
+	plansource->rewriteRowSecurity = row_security;
 
 	/*
 	 * Also save the current search_path in the query_context.  (This should
@@ -832,6 +810,13 @@ CheckCachedPlan(CachedPlanSource *plansource)
 	Assert(!plan->is_oneshot);
 
 	/*
+	 * If plan isn't valid for current role, we can't use it.
+	 */
+	if (plan->is_valid && plan->dependsOnRole &&
+		plan->planRoleId != GetUserId())
+		plan->is_valid = false;
+
+	/*
 	 * If it appears valid, acquire locks and recheck; this is much the same
 	 * logic as in RevalidateCachedQuery, but for a plan.
 	 */
@@ -900,6 +885,7 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
 	List	   *plist;
 	bool		snapshot_set;
 	bool		spi_pushed;
+	bool		is_transient;
 	MemoryContext plan_context;
 	MemoryContext oldcxt = CurrentMemoryContext;
 	ListCell   *lc;
@@ -997,7 +983,28 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
 	plan = (CachedPlan *) palloc(sizeof(CachedPlan));
 	plan->magic = CACHEDPLAN_MAGIC;
 	plan->stmt_list = plist;
-	if (plan_list_is_transient(plist))
+
+	/*
+	 * CachedPlan is dependent on role either if RLS affected the rewrite
+	 * phase or if a role dependency was injected during planning.  And it's
+	 * transient if any plan is marked so.
+	 */
+	plan->planRoleId = GetUserId();
+	plan->dependsOnRole = plansource->dependsOnRLS;
+	is_transient = false;
+	foreach(lc, plist)
+	{
+		PlannedStmt *plannedstmt = (PlannedStmt *) lfirst(lc);
+
+		if (!IsA(plannedstmt, PlannedStmt))
+			continue;			/* Ignore utility statements */
+
+		if (plannedstmt->transientPlan)
+			is_transient = true;
+		if (plannedstmt->dependsOnRole)
+			plan->dependsOnRole = true;
+	}
+	if (is_transient)
 	{
 		Assert(TransactionIdIsNormal(TransactionXmin));
 		plan->saved_xmin = TransactionXmin;
@@ -1010,20 +1017,6 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
 	plan->is_saved = false;
 	plan->is_valid = true;
 
-	/*
-	 * Walk through the plist and set hasForeignJoin if any of the plans have
-	 * it set.
-	 */
-	plan->has_foreign_join = false;
-	foreach(lc, plist)
-	{
-		PlannedStmt *plan_stmt = (PlannedStmt *) lfirst(lc);
-
-		if (IsA(plan_stmt, PlannedStmt))
-			plan->has_foreign_join =
-				plan->has_foreign_join || plan_stmt->hasForeignJoin;
-	}
-
 	/* assign generation number to new plan */
 	plan->generation = ++(plansource->generation);
 
@@ -1401,6 +1394,9 @@ CopyCachedPlan(CachedPlanSource *plansource)
 	if (plansource->search_path)
 		newsource->search_path = CopyOverrideSearchPath(plansource->search_path);
 	newsource->query_context = querytree_context;
+	newsource->rewriteRoleId = plansource->rewriteRoleId;
+	newsource->rewriteRowSecurity = plansource->rewriteRowSecurity;
+	newsource->dependsOnRLS = plansource->dependsOnRLS;
 
 	newsource->gplan = NULL;
 
@@ -1416,14 +1412,6 @@ CopyCachedPlan(CachedPlanSource *plansource)
 	newsource->total_custom_cost = plansource->total_custom_cost;
 	newsource->num_custom_plans = plansource->num_custom_plans;
 
-	/*
-	 * Copy over the user the query was planned as, and under what RLS
-	 * environment.  We will check during RevalidateCachedQuery() if the user
-	 * or environment has changed and, if so, will force a re-plan.
-	 */
-	newsource->planUserId = plansource->planUserId;
-	newsource->row_security_env = plansource->row_security_env;
-
 	MemoryContextSwitchTo(oldcxt);
 
 	return newsource;
@@ -1668,28 +1656,6 @@ ScanQueryWalker(Node *node, bool *acquire)
 }
 
 /*
- * plan_list_is_transient: check if any of the plans in the list are transient.
- */
-static bool
-plan_list_is_transient(List *stmt_list)
-{
-	ListCell   *lc;
-
-	foreach(lc, stmt_list)
-	{
-		PlannedStmt *plannedstmt = (PlannedStmt *) lfirst(lc);
-
-		if (!IsA(plannedstmt, PlannedStmt))
-			continue;			/* Ignore utility statements */
-
-		if (plannedstmt->transientPlan)
-			return true;
-	}
-
-	return false;
-}
-
-/*
  * PlanCacheComputeResultDesc: given a list of analyzed-and-rewritten Queries,
  * determine the result tupledesc it will produce.  Returns NULL if the
  * execution will not return tuples.
@@ -1888,40 +1854,6 @@ PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue)
 }
 
 /*
- * PlanCacheUserMappingCallback
- *		Syscache inval callback function for user mapping cache invalidation.
- *
- *	Invalidates plans which have pushed down foreign joins.
- */
-static void
-PlanCacheUserMappingCallback(Datum arg, int cacheid, uint32 hashvalue)
-{
-	CachedPlanSource *plansource;
-
-	for (plansource = first_saved_plan; plansource; plansource = plansource->next_saved)
-	{
-		Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);
-
-		/* No work if it's already invalidated */
-		if (!plansource->is_valid)
-			continue;
-
-		/* Never invalidate transaction control commands */
-		if (IsTransactionStmtPlan(plansource))
-			continue;
-
-		/*
-		 * If the plan has pushed down foreign joins, those join may become
-		 * unsafe to push down because of user mapping changes. Invalidate
-		 * only the generic plan, since changes to user mapping do not
-		 * invalidate the parse tree.
-		 */
-		if (plansource->gplan && plansource->gplan->has_foreign_join)
-			plansource->gplan->is_valid = false;
-	}
-}
-
-/*
  * ResetPlanCache: invalidate all cached plans.
  */
 void