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, 39 insertions, 56 deletions

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 93ebd8cab68..931bcfd37df 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -67,8 +67,6 @@ enum FdwScanPrivateIndex
 	FdwScanPrivateRetrievedAttrs,
 	/* Integer representing the desired fetch_size */
 	FdwScanPrivateFetchSize,
-	/* Oid of user mapping to be used while connecting to the foreign server */
-	FdwScanPrivateUserMappingOid,
 
 	/*
 	 * String describing join i.e. names of relations being joined and types
@@ -1226,11 +1224,10 @@ postgresGetForeignPlan(PlannerInfo *root,
 	 * Build the fdw_private list that will be available to the executor.
 	 * Items in the list must match order in enum FdwScanPrivateIndex.
 	 */
-	fdw_private = list_make5(makeString(sql.data),
+	fdw_private = list_make4(makeString(sql.data),
 							 remote_conds,
 							 retrieved_attrs,
-							 makeInteger(fpinfo->fetch_size),
-							 makeInteger(foreignrel->umid));
+							 makeInteger(fpinfo->fetch_size));
 	if (foreignrel->reloptkind == RELOPT_JOINREL)
 		fdw_private = lappend(fdw_private,
 							  makeString(fpinfo->relation_name->data));
@@ -1262,7 +1259,11 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
 	ForeignScan *fsplan = (ForeignScan *) node->ss.ps.plan;
 	EState	   *estate = node->ss.ps.state;
 	PgFdwScanState *fsstate;
+	RangeTblEntry *rte;
+	Oid			userid;
+	ForeignTable *table;
 	UserMapping *user;
+	int			rtindex;
 	int			numParams;
 
 	/*
@@ -1278,36 +1279,20 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
 	node->fdw_state = (void *) fsstate;
 
 	/*
-	 * Obtain the foreign server where to connect and user mapping to use for
-	 * connection. For base relations we obtain this information from
-	 * catalogs. For join relations, this information is frozen at the time of
-	 * planning to ensure that the join is safe to pushdown. In case the
-	 * information goes stale between planning and execution, plan will be
-	 * invalidated and replanned.
+	 * Identify which user to do the remote access as.  This should match what
+	 * ExecCheckRTEPerms() does.  In case of a join, use the lowest-numbered
+	 * member RTE as a representative; we would get the same result from any.
 	 */
 	if (fsplan->scan.scanrelid > 0)
-	{
-		ForeignTable *table;
-
-		/*
-		 * Identify which user to do the remote access as.  This should match
-		 * what ExecCheckRTEPerms() does.
-		 */
-		RangeTblEntry *rte = rt_fetch(fsplan->scan.scanrelid, estate->es_range_table);
-		Oid			userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
-
-		fsstate->rel = node->ss.ss_currentRelation;
-		table = GetForeignTable(RelationGetRelid(fsstate->rel));
-
-		user = GetUserMapping(userid, table->serverid);
-	}
+		rtindex = fsplan->scan.scanrelid;
 	else
-	{
-		Oid			umid = intVal(list_nth(fsplan->fdw_private, FdwScanPrivateUserMappingOid));
+		rtindex = bms_next_member(fsplan->fs_relids, -1);
+	rte = rt_fetch(rtindex, estate->es_range_table);
+	userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
 
-		user = GetUserMappingById(umid);
-		Assert(fsplan->fs_server == user->serverid);
-	}
+	/* Get info about foreign table. */
+	table = GetForeignTable(rte->relid);
+	user = GetUserMapping(userid, table->serverid);
 
 	/*
 	 * Get connection to the foreign server.  Connection manager will
@@ -1344,9 +1329,15 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
 	 * into local representation and error reporting during that process.
 	 */
 	if (fsplan->scan.scanrelid > 0)
+	{
+		fsstate->rel = node->ss.ss_currentRelation;
 		fsstate->tupdesc = RelationGetDescr(fsstate->rel);
+	}
 	else
+	{
+		fsstate->rel = NULL;
 		fsstate->tupdesc = node->ss.ss_ScanTupleSlot->tts_tupleDescriptor;
+	}
 
 	fsstate->attinmeta = TupleDescGetAttInMetadata(fsstate->tupdesc);
 
@@ -3966,16 +3957,6 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 	List	   *otherclauses;
 
 	/*
-	 * Core code may call GetForeignJoinPaths hook even when the join relation
-	 * doesn't have a valid user mapping associated with it. See
-	 * build_join_rel() for details. We can't push down such join, since there
-	 * doesn't exist a user mapping which can be used to connect to the
-	 * foreign server.
-	 */
-	if (!OidIsValid(joinrel->umid))
-		return false;
-
-	/*
 	 * We support pushing down INNER, LEFT, RIGHT and FULL OUTER joins.
 	 * Constructing queries representing SEMI and ANTI joins is hard, hence
 	 * not considered right now.
@@ -4151,6 +4132,20 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 	fpinfo->use_remote_estimate = fpinfo_o->use_remote_estimate ||
 		fpinfo_i->use_remote_estimate;
 
+	/* Get user mapping */
+	if (fpinfo->use_remote_estimate)
+	{
+		if (fpinfo_o->use_remote_estimate)
+			fpinfo->user = fpinfo_o->user;
+		else
+			fpinfo->user = fpinfo_i->user;
+	}
+	else
+		fpinfo->user = NULL;
+
+	/* Get foreign server */
+	fpinfo->server = fpinfo_o->server;
+
 	/*
 	 * Since both the joining relations come from the same server, the server
 	 * level options should have same value for both the relations. Pick from
@@ -4312,26 +4307,14 @@ postgresGetForeignJoinPaths(PlannerInfo *root,
 	cost_qual_eval(&fpinfo->local_conds_cost, fpinfo->local_conds, root);
 
 	/*
-	 * If we are going to estimate the costs using EXPLAIN, we will need
-	 * connection information. Fill it here.
+	 * If we are going to estimate costs locally, estimate the join clause
+	 * selectivity here while we have special join info.
 	 */
-	if (fpinfo->use_remote_estimate)
-		fpinfo->user = GetUserMappingById(joinrel->umid);
-	else
-	{
-		fpinfo->user = NULL;
-
-		/*
-		 * If we are going to estimate costs locally, estimate the join clause
-		 * selectivity here while we have special join info.
-		 */
+	if (!fpinfo->use_remote_estimate)
 		fpinfo->joinclause_sel = clauselist_selectivity(root, fpinfo->joinclauses,
 														0, fpinfo->jointype,
 														extra->sjinfo);
 
-	}
-	fpinfo->server = GetForeignServer(joinrel->serverid);
-
 	/* Estimate costs for bare join relation */
 	estimate_path_cost_size(root, joinrel, NIL, NIL, &rows,
 							&width, &startup_cost, &total_cost);