Skip to content

Commit fbe5a3f

Browse files
robertmhaasrobertmhaas
committed
Only try to push down foreign joins if the user mapping OIDs match.
Previously, the foreign join pushdown infrastructure left the question of security entirely up to individual FDWs, but it would be easy for a foreign data wrapper to inadvertently open up subtle security holes that way. So, make it the core code's job to determine which user mapping OID is relevant, and don't attempt join pushdown unless it's the same for all relevant relations. Per a suggestion from Tom Lane. Shigeru Hanada and Ashutosh Bapat, reviewed by Etsuro Fujita and KaiGai Kohei, with some further changes by me.
1 parent 2f6b041 commit fbe5a3f

File tree

13 files changed

+179
-20
lines changed

13 files changed

+179
-20
lines changed

‎src/backend/executor/execParallel.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ ExecSerializePlan(Plan *plan, EState *estate)
143143
pstmt->relationOids = NIL;
144144
pstmt->invalItems = NIL; /* workers can't replan anyway... */
145145
pstmt->hasRowSecurity = false;
146+
pstmt->hasForeignJoin = false;
146147

147148
/* Return serialized copy of our dummy PlannedStmt. */
148149
return nodeToString(pstmt);

‎src/backend/foreign/foreign.c

Lines changed: 57 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
extern Datum pg_options_to_table(PG_FUNCTION_ARGS);
3232
extern Datum postgresql_fdw_validator(PG_FUNCTION_ARGS);
3333

34+
static HeapTuple find_user_mapping(Oid userid, Oid serverid);
3435

3536
/*
3637
* GetForeignDataWrapper - look up the foreign-data wrapper by OID.
@@ -174,23 +175,7 @@ GetUserMapping(Oid userid, Oid serverid)
174175
bool isnull;
175176
UserMapping *um;
176177

177-
tp = SearchSysCache2(USERMAPPINGUSERSERVER,
178-
ObjectIdGetDatum(userid),
179-
ObjectIdGetDatum(serverid));
180-
181-
if (!HeapTupleIsValid(tp))
182-
{
183-
/* Not found for the specific user -- try PUBLIC */
184-
tp = SearchSysCache2(USERMAPPINGUSERSERVER,
185-
ObjectIdGetDatum(InvalidOid),
186-
ObjectIdGetDatum(serverid));
187-
}
188-
189-
if (!HeapTupleIsValid(tp))
190-
ereport(ERROR,
191-
(errcode(ERRCODE_UNDEFINED_OBJECT),
192-
errmsg("user mapping not found for \"%s\"",
193-
MappingUserName(userid))));
178+
tp = find_user_mapping(userid, serverid);
194179

195180
um = (UserMapping *) palloc(sizeof(UserMapping));
196181
um->umid = HeapTupleGetOid(tp);
@@ -212,6 +197,61 @@ GetUserMapping(Oid userid, Oid serverid)
212197
return um;
213198
}
214199

200+
/*
201+
* GetUserMappingId - look up the user mapping, and return its OID
202+
*
203+
* If no mapping is found for the supplied user, we also look for
204+
* PUBLIC mappings (userid == InvalidOid).
205+
*/
206+
Oid
207+
GetUserMappingId(Oid userid, Oid serverid)
208+
{
209+
HeapTuple tp;
210+
Oid umid;
211+
212+
tp = find_user_mapping(userid, serverid);
213+
214+
/* Extract the Oid */
215+
umid = HeapTupleGetOid(tp);
216+
217+
ReleaseSysCache(tp);
218+
219+
return umid;
220+
}
221+
222+
223+
/*
224+
* find_user_mapping - Guts of GetUserMapping family.
225+
*
226+
* If no mapping is found for the supplied user, we also look for
227+
* PUBLIC mappings (userid == InvalidOid).
228+
*/
229+
static HeapTuple
230+
find_user_mapping(Oid userid, Oid serverid)
231+
{
232+
HeapTuple tp;
233+
234+
tp = SearchSysCache2(USERMAPPINGUSERSERVER,
235+
ObjectIdGetDatum(userid),
236+
ObjectIdGetDatum(serverid));
237+
238+
if (HeapTupleIsValid(tp))
239+
return tp;
240+
241+
/* Not found for the specific user -- try PUBLIC */
242+
tp = SearchSysCache2(USERMAPPINGUSERSERVER,
243+
ObjectIdGetDatum(InvalidOid),
244+
ObjectIdGetDatum(serverid));
245+
246+
if (!HeapTupleIsValid(tp))
247+
ereport(ERROR,
248+
(errcode(ERRCODE_UNDEFINED_OBJECT),
249+
errmsg("user mapping not found for \"%s\"",
250+
MappingUserName(userid))));
251+
252+
return tp;
253+
}
254+
215255

216256
/*
217257
* GetForeignTable - look up the foreign table definition by relation oid.

‎src/backend/nodes/copyfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ _copyPlannedStmt(const PlannedStmt *from)
9595
COPY_SCALAR_FIELD(nParamExec);
9696
COPY_SCALAR_FIELD(hasRowSecurity);
9797
COPY_SCALAR_FIELD(parallelModeNeeded);
98+
COPY_SCALAR_FIELD(hasForeignJoin);
9899

99100
return newnode;
100101
}

‎src/backend/nodes/outfuncs.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ _outPlannedStmt(StringInfo str, const PlannedStmt *node)
259259
WRITE_INT_FIELD(nParamExec);
260260
WRITE_BOOL_FIELD(hasRowSecurity);
261261
WRITE_BOOL_FIELD(parallelModeNeeded);
262+
WRITE_BOOL_FIELD(hasForeignJoin);
262263
}
263264

264265
/*
@@ -1825,6 +1826,7 @@ _outPlannerGlobal(StringInfo str, const PlannerGlobal *node)
18251826
WRITE_BOOL_FIELD(hasRowSecurity);
18261827
WRITE_BOOL_FIELD(parallelModeOK);
18271828
WRITE_BOOL_FIELD(parallelModeNeeded);
1829+
WRITE_BOOL_FIELD(hasForeignJoin);
18281830
}
18291831

18301832
static void

‎src/backend/nodes/readfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1396,6 +1396,7 @@ _readPlannedStmt(void)
13961396
READ_INT_FIELD(nParamExec);
13971397
READ_BOOL_FIELD(hasRowSecurity);
13981398
READ_BOOL_FIELD(parallelModeNeeded);
1399+
READ_BOOL_FIELD(hasForeignJoin);
13991400

14001401
READ_DONE();
14011402
}

‎src/backend/optimizer/plan/createplan.c

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2151,6 +2151,15 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
21512151
/* Likewise, copy the relids that are represented by this foreign scan */
21522152
scan_plan->fs_relids = best_path->path.parent->relids;
21532153

2154+
/*
2155+
* If a join between foreign relations was pushed down, remember it. The
2156+
* push-down safety of the join depends upon the server and user mapping
2157+
* being same. That can change between planning and execution time, in which
2158+
* case the plan should be invalidated.
2159+
*/
2160+
if (scan_relid == 0)
2161+
root->glob->hasForeignJoin = true;
2162+
21542163
/*
21552164
* Replace any outer-relation variables with nestloop params in the qual,
21562165
* fdw_exprs and fdw_recheck_quals expressions. We do this last so that

‎src/backend/optimizer/plan/planner.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,7 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
200200
glob->lastPlanNodeId = 0;
201201
glob->transientPlan = false;
202202
glob->hasRowSecurity = false;
203+
glob->hasForeignJoin = false;
203204

204205
/*
205206
* Assess whether it's feasible to use parallel mode for this query. We
@@ -346,6 +347,7 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
346347
result->nParamExec = glob->nParamExec;
347348
result->hasRowSecurity = glob->hasRowSecurity;
348349
result->parallelModeNeeded = glob->parallelModeNeeded;
350+
result->hasForeignJoin = glob->hasForeignJoin;
349351

350352
return result;
351353
}

‎src/backend/optimizer/util/relnode.c

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@
1414
*/
1515
#include "postgres.h"
1616

17+
#include "miscadmin.h"
18+
#include "catalog/pg_class.h"
19+
#include "foreign/foreign.h"
1720
#include "optimizer/clauses.h"
1821
#include "optimizer/cost.h"
1922
#include "optimizer/pathnode.h"
@@ -127,6 +130,7 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptKind reloptkind)
127130
rel->subroot = NULL;
128131
rel->subplan_params = NIL;
129132
rel->serverid = InvalidOid;
133+
rel->umid = InvalidOid;
130134
rel->fdwroutine = NULL;
131135
rel->fdw_private = NULL;
132136
rel->baserestrictinfo = NIL;
@@ -166,6 +170,26 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptKind reloptkind)
166170
break;
167171
}
168172

173+
/* For foreign tables get the user mapping */
174+
if (rte->relkind == RELKIND_FOREIGN_TABLE)
175+
{
176+
/*
177+
* This should match what ExecCheckRTEPerms() does.
178+
*
179+
* Note that if the plan ends up depending on the user OID in any
180+
* way - e.g. if it depends on the computed user mapping OID - we must
181+
* ensure that it gets invalidated in the case of a user OID change.
182+
* See RevalidateCachedQuery and more generally the hasForeignJoin
183+
* flags in PlannerGlobal and PlannedStmt.
184+
*/
185+
Oid userid;
186+
187+
userid = OidIsValid(rte->checkAsUser) ? rte->checkAsUser : GetUserId();
188+
rel->umid = GetUserMappingId(userid, rel->serverid);
189+
}
190+
else
191+
rel->umid = InvalidOid;
192+
169193
/* Save the finished struct in the query's simple_rel_array */
170194
root->simple_rel_array[relid] = rel;
171195

@@ -398,6 +422,7 @@ build_join_rel(PlannerInfo *root,
398422
joinrel->subroot = NULL;
399423
joinrel->subplan_params = NIL;
400424
joinrel->serverid = InvalidOid;
425+
joinrel->umid = InvalidOid;
401426
joinrel->fdwroutine = NULL;
402427
joinrel->fdw_private = NULL;
403428
joinrel->baserestrictinfo = NIL;
@@ -408,12 +433,19 @@ build_join_rel(PlannerInfo *root,
408433

409434
/*
410435
* Set up foreign-join fields if outer and inner relation are foreign
411-
* tables (or joins) belonging to the same server.
436+
* tables (or joins) belonging to the same server and using the same
437+
* user mapping.
438+
*
439+
* Otherwise those fields are left invalid, so FDW API will not be called
440+
* for the join relation.
412441
*/
413442
if (OidIsValid(outer_rel->serverid) &&
414-
inner_rel->serverid == outer_rel->serverid)
443+
inner_rel->serverid == outer_rel->serverid &&
444+
inner_rel->umid == outer_rel->umid)
415445
{
446+
Assert(OidIsValid(outer_rel->umid));
416447
joinrel->serverid = outer_rel->serverid;
448+
joinrel->umid = outer_rel->umid;
417449
joinrel->fdwroutine = outer_rel->fdwroutine;
418450
}
419451

‎src/backend/utils/cache/plancache.c

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,8 @@ static TupleDesc PlanCacheComputeResultDesc(List *stmt_list);
104104
static void PlanCacheRelCallback(Datum arg, Oid relid);
105105
static void PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue);
106106
static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
107+
static void PlanCacheUserMappingCallback(Datum arg, int cacheid,
108+
uint32 hashvalue);
107109

108110

109111
/*
@@ -119,6 +121,8 @@ InitPlanCache(void)
119121
CacheRegisterSyscacheCallback(NAMESPACEOID, PlanCacheSysCallback, (Datum) 0);
120122
CacheRegisterSyscacheCallback(OPEROID, PlanCacheSysCallback, (Datum) 0);
121123
CacheRegisterSyscacheCallback(AMOPOPID, PlanCacheSysCallback, (Datum) 0);
124+
/* User mapping change may invalidate plans with pushed down foreign join */
125+
CacheRegisterSyscacheCallback(USERMAPPINGOID, PlanCacheUserMappingCallback, (Datum) 0);
122126
}
123127

124128
/*
@@ -574,7 +578,8 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
574578
/*
575579
* If this is a new cached plan, then set the user id it was planned by
576580
* and under what row security settings; these are needed to determine
577-
* plan invalidation when RLS is involved.
581+
* plan invalidation when RLS is involved or foreign joins are pushed
582+
* down.
578583
*/
579584
if (!OidIsValid(plansource->planUserId))
580585
{
@@ -609,6 +614,18 @@ RevalidateCachedQuery(CachedPlanSource *plansource)
609614
|| plansource->row_security_env != row_security))
610615
plansource->is_valid = false;
611616

617+
/*
618+
* If we have a join pushed down to the foreign server and the current user
619+
* is different from the one for which the plan was created, invalidate the
620+
* generic plan since user mapping for the new user might make the join
621+
* unsafe to push down, or change which user mapping is used.
622+
*/
623+
if (plansource->is_valid &&
624+
plansource->gplan &&
625+
plansource->gplan->has_foreign_join &&
626+
plansource->planUserId != GetUserId())
627+
plansource->gplan->is_valid = false;
628+
612629
/*
613630
* If the query is currently valid, acquire locks on the referenced
614631
* objects; then check again. We need to do it this way to cover the race
@@ -881,6 +898,7 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
881898
bool spi_pushed;
882899
MemoryContext plan_context;
883900
MemoryContext oldcxt = CurrentMemoryContext;
901+
ListCell *lc;
884902

885903
/*
886904
* Normally the querytree should be valid already, but if it's not,
@@ -988,6 +1006,20 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
9881006
plan->is_saved = false;
9891007
plan->is_valid = true;
9901008

1009+
/*
1010+
* Walk through the plist and set hasForeignJoin if any of the plans have
1011+
* it set.
1012+
*/
1013+
plan->has_foreign_join = false;
1014+
foreach(lc, plist)
1015+
{
1016+
PlannedStmt *plan_stmt = (PlannedStmt *) lfirst(lc);
1017+
1018+
if (IsA(plan_stmt, PlannedStmt))
1019+
plan->has_foreign_join =
1020+
plan->has_foreign_join || plan_stmt->hasForeignJoin;
1021+
}
1022+
9911023
/* assign generation number to new plan */
9921024
plan->generation = ++(plansource->generation);
9931025

@@ -1843,6 +1875,40 @@ PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue)
18431875
ResetPlanCache();
18441876
}
18451877

1878+
/*
1879+
* PlanCacheUserMappingCallback
1880+
* Syscache inval callback function for user mapping cache invalidation.
1881+
*
1882+
* Invalidates plans which have pushed down foreign joins.
1883+
*/
1884+
static void
1885+
PlanCacheUserMappingCallback(Datum arg, int cacheid, uint32 hashvalue)
1886+
{
1887+
CachedPlanSource *plansource;
1888+
1889+
for (plansource = first_saved_plan; plansource; plansource = plansource->next_saved)
1890+
{
1891+
Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);
1892+
1893+
/* No work if it's already invalidated */
1894+
if (!plansource->is_valid)
1895+
continue;
1896+
1897+
/* Never invalidate transaction control commands */
1898+
if (IsTransactionStmtPlan(plansource))
1899+
continue;
1900+
1901+
/*
1902+
* If the plan has pushed down foreign joins, those join may become
1903+
* unsafe to push down because of user mapping changes. Invalidate only
1904+
* the generic plan, since changes to user mapping do not invalidate the
1905+
* parse tree.
1906+
*/
1907+
if (plansource->gplan && plansource->gplan->has_foreign_join)
1908+
plansource->gplan->is_valid = false;
1909+
}
1910+
}
1911+
18461912
/*
18471913
* ResetPlanCache: invalidate all cached plans.
18481914
*/

‎src/include/foreign/foreign.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ typedef struct ForeignTable
7272
extern ForeignServer *GetForeignServer(Oid serverid);
7373
extern ForeignServer *GetForeignServerByName(const char *name, bool missing_ok);
7474
extern UserMapping *GetUserMapping(Oid userid, Oid serverid);
75+
extern Oid GetUserMappingId(Oid userid, Oid serverid);
7576
extern ForeignDataWrapper *GetForeignDataWrapper(Oid fdwid);
7677
extern ForeignDataWrapper *GetForeignDataWrapperByName(const char *name,
7778
bool missing_ok);

‎src/include/nodes/plannodes.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ typedef struct PlannedStmt
7373
bool hasRowSecurity; /* row security applied? */
7474

7575
bool parallelModeNeeded; /* parallel mode required to execute? */
76+
bool hasForeignJoin; /* Plan has a pushed down foreign join */
7677
} PlannedStmt;
7778

7879
/* macro for fetching the Plan associated with a SubPlan node */

‎src/include/nodes/relation.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ typedef struct PlannerGlobal
108108
bool parallelModeOK; /* parallel mode potentially OK? */
109109

110110
bool parallelModeNeeded; /* parallel mode actually required? */
111+
bool hasForeignJoin; /* does have a pushed down foreign join */
111112
} PlannerGlobal;
112113

113114
/* macro for fetching the Plan associated with a SubPlan node */
@@ -490,6 +491,7 @@ typedef struct RelOptInfo
490491

491492
/* Information about foreign tables and foreign joins */
492493
Oid serverid; /* identifies server for the table or join */
494+
Oid umid; /* identifies user mapping for the table or join */
493495
/* use "struct FdwRoutine" to avoid including fdwapi.h here */
494496
struct FdwRoutine *fdwroutine;
495497
void *fdw_private;

‎src/include/utils/plancache.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ typedef struct CachedPlan
135135
* changes from this value */
136136
int generation; /* parent's generation number for this plan */
137137
int refcount; /* count of live references to this struct */
138+
bool has_foreign_join; /* plan has pushed down a foreign join */
138139
MemoryContext context; /* context containing this CachedPlan */
139140
} CachedPlan;
140141

0 commit comments

Comments
 (0)