Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 45639a0

Browse files
committed
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 fbe5a3f and 5d4171d, 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 parent 533e9c6 commit 45639a0

File tree

19 files changed

+579
-716
lines changed

19 files changed

+579
-716
lines changed

contrib/postgres_fdw/expected/postgres_fdw.out

+240-252
Large diffs are not rendered by default.

contrib/postgres_fdw/postgres_fdw.c

+39-56
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,6 @@ enum FdwScanPrivateIndex
6767
FdwScanPrivateRetrievedAttrs,
6868
/* Integer representing the desired fetch_size */
6969
FdwScanPrivateFetchSize,
70-
/* Oid of user mapping to be used while connecting to the foreign server */
71-
FdwScanPrivateUserMappingOid,
7270

7371
/*
7472
* String describing join i.e. names of relations being joined and types
@@ -1226,11 +1224,10 @@ postgresGetForeignPlan(PlannerInfo *root,
12261224
* Build the fdw_private list that will be available to the executor.
12271225
* Items in the list must match order in enum FdwScanPrivateIndex.
12281226
*/
1229-
fdw_private = list_make5(makeString(sql.data),
1227+
fdw_private = list_make4(makeString(sql.data),
12301228
remote_conds,
12311229
retrieved_attrs,
1232-
makeInteger(fpinfo->fetch_size),
1233-
makeInteger(foreignrel->umid));
1230+
makeInteger(fpinfo->fetch_size));
12341231
if (foreignrel->reloptkind == RELOPT_JOINREL)
12351232
fdw_private = lappend(fdw_private,
12361233
makeString(fpinfo->relation_name->data));
@@ -1262,7 +1259,11 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
12621259
ForeignScan *fsplan = (ForeignScan *) node->ss.ps.plan;
12631260
EState *estate = node->ss.ps.state;
12641261
PgFdwScanState *fsstate;
1262+
RangeTblEntry *rte;
1263+
Oid userid;
1264+
ForeignTable *table;
12651265
UserMapping *user;
1266+
int rtindex;
12661267
int numParams;
12671268

12681269
/*
@@ -1278,36 +1279,20 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
12781279
node->fdw_state = (void *) fsstate;
12791280

12801281
/*
1281-
* Obtain the foreign server where to connect and user mapping to use for
1282-
* connection. For base relations we obtain this information from
1283-
* catalogs. For join relations, this information is frozen at the time of
1284-
* planning to ensure that the join is safe to pushdown. In case the
1285-
* information goes stale between planning and execution, plan will be
1286-
* invalidated and replanned.
1282+
* Identify which user to do the remote access as. This should match what
1283+
* ExecCheckRTEPerms() does. In case of a join, use the lowest-numbered
1284+
* member RTE as a representative; we would get the same result from any.
12871285
*/
12881286
if (fsplan->scan.scanrelid > 0)
1289-
{
1290-
ForeignTable *table;
1291-
1292-
/*
1293-
* Identify which user to do the remote access as. This should match
1294-
* what ExecCheckRTEPerms() does.
1295-
*/
1296-
RangeTblEntry *rte = rt_fetch(fsplan->scan.scanrelid, estate->es_range_table);
1297-
Oid userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
1298-
1299-
fsstate->rel = node->ss.ss_currentRelation;
1300-
table = GetForeignTable(RelationGetRelid(fsstate->rel));
1301-
1302-
user = GetUserMapping(userid, table->serverid);
1303-
}
1287+
rtindex = fsplan->scan.scanrelid;
13041288
else
1305-
{
1306-
Oid umid = intVal(list_nth(fsplan->fdw_private, FdwScanPrivateUserMappingOid));
1289+
rtindex = bms_next_member(fsplan->fs_relids, -1);
1290+
rte = rt_fetch(rtindex, estate->es_range_table);
1291+
userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
13071292

1308-
user = GetUserMappingById(umid);
1309-
Assert(fsplan->fs_server == user->serverid);
1310-
}
1293+
/* Get info about foreign table. */
1294+
table = GetForeignTable(rte->relid);
1295+
user = GetUserMapping(userid, table->serverid);
13111296

13121297
/*
13131298
* Get connection to the foreign server. Connection manager will
@@ -1344,9 +1329,15 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags)
13441329
* into local representation and error reporting during that process.
13451330
*/
13461331
if (fsplan->scan.scanrelid > 0)
1332+
{
1333+
fsstate->rel = node->ss.ss_currentRelation;
13471334
fsstate->tupdesc = RelationGetDescr(fsstate->rel);
1335+
}
13481336
else
1337+
{
1338+
fsstate->rel = NULL;
13491339
fsstate->tupdesc = node->ss.ss_ScanTupleSlot->tts_tupleDescriptor;
1340+
}
13501341

13511342
fsstate->attinmeta = TupleDescGetAttInMetadata(fsstate->tupdesc);
13521343

@@ -3965,16 +3956,6 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
39653956
List *joinclauses;
39663957
List *otherclauses;
39673958

3968-
/*
3969-
* Core code may call GetForeignJoinPaths hook even when the join relation
3970-
* doesn't have a valid user mapping associated with it. See
3971-
* build_join_rel() for details. We can't push down such join, since there
3972-
* doesn't exist a user mapping which can be used to connect to the
3973-
* foreign server.
3974-
*/
3975-
if (!OidIsValid(joinrel->umid))
3976-
return false;
3977-
39783959
/*
39793960
* We support pushing down INNER, LEFT, RIGHT and FULL OUTER joins.
39803961
* Constructing queries representing SEMI and ANTI joins is hard, hence
@@ -4151,6 +4132,20 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
41514132
fpinfo->use_remote_estimate = fpinfo_o->use_remote_estimate ||
41524133
fpinfo_i->use_remote_estimate;
41534134

4135+
/* Get user mapping */
4136+
if (fpinfo->use_remote_estimate)
4137+
{
4138+
if (fpinfo_o->use_remote_estimate)
4139+
fpinfo->user = fpinfo_o->user;
4140+
else
4141+
fpinfo->user = fpinfo_i->user;
4142+
}
4143+
else
4144+
fpinfo->user = NULL;
4145+
4146+
/* Get foreign server */
4147+
fpinfo->server = fpinfo_o->server;
4148+
41544149
/*
41554150
* Since both the joining relations come from the same server, the server
41564151
* level options should have same value for both the relations. Pick from
@@ -4312,26 +4307,14 @@ postgresGetForeignJoinPaths(PlannerInfo *root,
43124307
cost_qual_eval(&fpinfo->local_conds_cost, fpinfo->local_conds, root);
43134308

43144309
/*
4315-
* If we are going to estimate the costs using EXPLAIN, we will need
4316-
* connection information. Fill it here.
4310+
* If we are going to estimate costs locally, estimate the join clause
4311+
* selectivity here while we have special join info.
43174312
*/
4318-
if (fpinfo->use_remote_estimate)
4319-
fpinfo->user = GetUserMappingById(joinrel->umid);
4320-
else
4321-
{
4322-
fpinfo->user = NULL;
4323-
4324-
/*
4325-
* If we are going to estimate costs locally, estimate the join clause
4326-
* selectivity here while we have special join info.
4327-
*/
4313+
if (!fpinfo->use_remote_estimate)
43284314
fpinfo->joinclause_sel = clauselist_selectivity(root, fpinfo->joinclauses,
43294315
0, fpinfo->jointype,
43304316
extra->sjinfo);
43314317

4332-
}
4333-
fpinfo->server = GetForeignServer(joinrel->serverid);
4334-
43354318
/* Estimate costs for bare join relation */
43364319
estimate_path_cost_size(root, joinrel, NIL, NIL, &rows,
43374320
&width, &startup_cost, &total_cost);

0 commit comments

Comments
 (0)