From 5d4171d1c70edfe3e9be1de9e66603af28e3afe1 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 28 Mar 2016 21:50:28 -0400 Subject: [PATCH] Don't require a user mapping for FDWs to work. Commit fbe5a3fb73102c2cfec11aaaa4a67943f4474383 accidentally changed this behavior; put things back the way they were, and add some regression tests. Report by Andres Freund; patch by Ashutosh Bapat, with a bit of kibitzing by me. --- contrib/file_fdw/input/file_fdw.source | 3 + contrib/file_fdw/output/file_fdw.source | 14 ++++- .../postgres_fdw/expected/postgres_fdw.out | 61 +++++++++++++++++-- contrib/postgres_fdw/postgres_fdw.c | 10 +++ contrib/postgres_fdw/sql/postgres_fdw.sql | 9 ++- src/backend/foreign/foreign.c | 36 ++++++++--- src/backend/optimizer/util/relnode.c | 12 +++- src/include/foreign/foreign.h | 2 +- 8 files changed, 125 insertions(+), 22 deletions(-) diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source index 416753dcad..35db4af082 100644 --- a/contrib/file_fdw/input/file_fdw.source +++ b/contrib/file_fdw/input/file_fdw.source @@ -173,6 +173,9 @@ SET ROLE file_fdw_user; \t on EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_text WHERE a > 0; \t off +-- file FDW allows foreign tables to be accessed without user mapping +DROP USER MAPPING FOR file_fdw_user SERVER file_server; +SELECT * FROM agg_text ORDER BY a; -- privilege tests for object SET ROLE file_fdw_superuser; diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source index 8719694276..26f4999cd1 100644 --- a/contrib/file_fdw/output/file_fdw.source +++ b/contrib/file_fdw/output/file_fdw.source @@ -322,6 +322,17 @@ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_text WHERE a > 0; Foreign File: @abs_srcdir@/data/agg.data \t off +-- file FDW allows foreign tables to be accessed without user mapping +DROP USER MAPPING FOR file_fdw_user SERVER file_server; +SELECT * FROM agg_text ORDER BY a; + a | b +-----+--------- + 0 | 0.09561 + 42 | 324.78 + 56 | 7.8 + 100 | 99.097 +(4 rows) + -- privilege tests for object SET ROLE file_fdw_superuser; ALTER FOREIGN TABLE agg_text OWNER TO file_fdw_user; @@ -333,9 +344,8 @@ SET ROLE file_fdw_superuser; -- cleanup RESET ROLE; DROP EXTENSION file_fdw CASCADE; -NOTICE: drop cascades to 8 other objects +NOTICE: drop cascades to 7 other objects DETAIL: drop cascades to server file_server -drop cascades to user mapping for file_fdw_user on server file_server drop cascades to user mapping for file_fdw_superuser on server file_server drop cascades to user mapping for no_priv_user on server file_server drop cascades to foreign table agg_text diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 96535d4180..50f1261e63 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -1958,13 +1958,30 @@ EXECUTE join_stmt; -- change the session user to view_owner and execute the statement. Because of -- change in session user, the plan should get invalidated and created again. --- While creating the plan, it should throw error since there is no user mapping --- available for view_owner. +-- The join will not be pushed down since the joining relations do not have a +-- valid user mapping. SET SESSION ROLE view_owner; EXPLAIN (COSTS OFF, VERBOSE) EXECUTE join_stmt; -ERROR: user mapping not found for "view_owner" -EXECUTE join_stmt; -ERROR: user mapping not found for "view_owner" + QUERY PLAN +------------------------------------------------------------------ + Limit + Output: t1.c1, t2.c1 + -> Sort + Output: t1.c1, t2.c1 + Sort Key: t1.c1, t2.c1 + -> Hash Left Join + Output: t1.c1, t2.c1 + Hash Cond: (t1.c1 = t2.c1) + -> Foreign Scan on public.ft4 t1 + Output: t1.c1, t1.c2, t1.c3 + Remote SQL: SELECT c1 FROM "S 1"."T 3" + -> Hash + Output: t2.c1 + -> Foreign Scan on public.ft5 t2 + Output: t2.c1 + Remote SQL: SELECT c1 FROM "S 1"."T 4" +(16 rows) + RESET ROLE; DEALLOCATE join_stmt; CREATE VIEW v_ft5 AS SELECT * FROM ft5; @@ -2021,6 +2038,40 @@ EXECUTE join_stmt; ----+---- (0 rows) +-- If a sub-join can't be pushed down, upper level join shouldn't be either. +EXPLAIN (COSTS false, VERBOSE) +SELECT t1.c1, t2.c1 FROM (ft5 t1 JOIN v_ft5 t2 ON (t1.c1 = t2.c1)) left join (ft5 t3 JOIN v_ft5 t4 ON (t3.c1 = t4.c1)) ON (t1.c1 = t3.c1); + QUERY PLAN +------------------------------------------------------------------ + Hash Join + Output: t1.c1, ft5.c1 + Hash Cond: (t1.c1 = ft5.c1) + -> Hash Right Join + Output: t1.c1 + Hash Cond: (t3.c1 = t1.c1) + -> Hash Join + Output: t3.c1 + Hash Cond: (t3.c1 = ft5_1.c1) + -> Foreign Scan on public.ft5 t3 + Output: t3.c1, t3.c2, t3.c3 + Remote SQL: SELECT c1 FROM "S 1"."T 4" + -> Hash + Output: ft5_1.c1 + -> Foreign Scan on public.ft5 ft5_1 + Output: ft5_1.c1 + Remote SQL: SELECT c1 FROM "S 1"."T 4" + -> Hash + Output: t1.c1 + -> Foreign Scan on public.ft5 t1 + Output: t1.c1 + Remote SQL: SELECT c1 FROM "S 1"."T 4" + -> Hash + Output: ft5.c1 + -> Foreign Scan on public.ft5 + Output: ft5.c1 + Remote SQL: SELECT c1 FROM "S 1"."T 4" +(27 rows) + -- recreate the dropped user mapping for further tests CREATE USER MAPPING FOR CURRENT_USER SERVER loopback; DROP USER MAPPING FOR PUBLIC SERVER loopback; diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index f21689e73d..4fbbde13bc 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -3910,6 +3910,16 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype, List *joinclauses; 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 diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 61cbf55ab9..f420b230e7 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -478,11 +478,10 @@ EXPLAIN (COSTS OFF, VERBOSE) EXECUTE join_stmt; EXECUTE join_stmt; -- change the session user to view_owner and execute the statement. Because of -- change in session user, the plan should get invalidated and created again. --- While creating the plan, it should throw error since there is no user mapping --- available for view_owner. +-- The join will not be pushed down since the joining relations do not have a +-- valid user mapping. SET SESSION ROLE view_owner; EXPLAIN (COSTS OFF, VERBOSE) EXECUTE join_stmt; -EXECUTE join_stmt; RESET ROLE; DEALLOCATE join_stmt; @@ -506,6 +505,10 @@ CREATE USER MAPPING FOR view_owner SERVER loopback; EXPLAIN (COSTS false, VERBOSE) EXECUTE join_stmt; EXECUTE join_stmt; +-- If a sub-join can't be pushed down, upper level join shouldn't be either. +EXPLAIN (COSTS false, VERBOSE) +SELECT t1.c1, t2.c1 FROM (ft5 t1 JOIN v_ft5 t2 ON (t1.c1 = t2.c1)) left join (ft5 t3 JOIN v_ft5 t4 ON (t3.c1 = t4.c1)) ON (t1.c1 = t3.c1); + -- recreate the dropped user mapping for further tests CREATE USER MAPPING FOR CURRENT_USER SERVER loopback; DROP USER MAPPING FOR PUBLIC SERVER loopback; diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index 239849bb0b..f1feb85c55 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -31,7 +31,7 @@ extern Datum pg_options_to_table(PG_FUNCTION_ARGS); extern Datum postgresql_fdw_validator(PG_FUNCTION_ARGS); -static HeapTuple find_user_mapping(Oid userid, Oid serverid); +static HeapTuple find_user_mapping(Oid userid, Oid serverid, bool missing_ok); /* * GetForeignDataWrapper - look up the foreign-data wrapper by OID. @@ -223,7 +223,7 @@ GetUserMapping(Oid userid, Oid serverid) bool isnull; UserMapping *um; - tp = find_user_mapping(userid, serverid); + tp = find_user_mapping(userid, serverid, false); um = (UserMapping *) palloc(sizeof(UserMapping)); um->umid = HeapTupleGetOid(tp); @@ -250,14 +250,23 @@ GetUserMapping(Oid userid, Oid serverid) * * If no mapping is found for the supplied user, we also look for * PUBLIC mappings (userid == InvalidOid). + * + * If missing_ok is true, the function returns InvalidOid when it does not find + * required user mapping. Otherwise, find_user_mapping() throws error if it + * does not find required user mapping. */ Oid -GetUserMappingId(Oid userid, Oid serverid) +GetUserMappingId(Oid userid, Oid serverid, bool missing_ok) { HeapTuple tp; Oid umid; - tp = find_user_mapping(userid, serverid); + tp = find_user_mapping(userid, serverid, missing_ok); + + Assert(missing_ok || tp); + + if (!tp && missing_ok) + return InvalidOid; /* Extract the Oid */ umid = HeapTupleGetOid(tp); @@ -273,9 +282,13 @@ GetUserMappingId(Oid userid, Oid serverid) * * If no mapping is found for the supplied user, we also look for * PUBLIC mappings (userid == InvalidOid). + * + * If missing_ok is true, the function returns NULL, if it does not find + * the required user mapping. Otherwise, it throws error if it does not + * find the required user mapping. */ static HeapTuple -find_user_mapping(Oid userid, Oid serverid) +find_user_mapping(Oid userid, Oid serverid, bool missing_ok) { HeapTuple tp; @@ -292,10 +305,15 @@ find_user_mapping(Oid userid, Oid serverid) ObjectIdGetDatum(serverid)); if (!HeapTupleIsValid(tp)) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("user mapping not found for \"%s\"", - MappingUserName(userid)))); + { + if (missing_ok) + return NULL; + else + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("user mapping not found for \"%s\"", + MappingUserName(userid)))); + } return tp; } diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c index 7e37edf5f5..6f24b031e4 100644 --- a/src/backend/optimizer/util/relnode.c +++ b/src/backend/optimizer/util/relnode.c @@ -180,11 +180,15 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptKind reloptkind) * ensure that it gets invalidated in the case of a user OID change. * See RevalidateCachedQuery and more generally the hasForeignJoin * flags in PlannerGlobal and PlannedStmt. + * + * It's possible, and not necessarily an error, for rel->umid to be + * InvalidOid even though rel->serverid is set. That just means there + * is a server with no user mapping. */ Oid userid; userid = OidIsValid(rte->checkAsUser) ? rte->checkAsUser : GetUserId(); - rel->umid = GetUserMappingId(userid, rel->serverid); + rel->umid = GetUserMappingId(userid, rel->serverid, true); } else rel->umid = InvalidOid; @@ -435,12 +439,16 @@ build_join_rel(PlannerInfo *root, * * Otherwise those fields are left invalid, so FDW API will not be called * for the join relation. + * + * For FDWs like file_fdw, which ignore user mapping, the user mapping id + * associated with the joining relation may be invalid. A valid serverid + * distinguishes between a pushed down join with no user mapping and + * a join which can not be pushed down because of user mapping mismatch. */ if (OidIsValid(outer_rel->serverid) && inner_rel->serverid == outer_rel->serverid && inner_rel->umid == outer_rel->umid) { - Assert(OidIsValid(outer_rel->umid)); joinrel->serverid = outer_rel->serverid; joinrel->umid = outer_rel->umid; joinrel->fdwroutine = outer_rel->fdwroutine; diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h index 71f8e55b0e..fb945e9ffd 100644 --- a/src/include/foreign/foreign.h +++ b/src/include/foreign/foreign.h @@ -72,7 +72,7 @@ typedef struct ForeignTable extern ForeignServer *GetForeignServer(Oid serverid); extern ForeignServer *GetForeignServerByName(const char *name, bool missing_ok); extern UserMapping *GetUserMapping(Oid userid, Oid serverid); -extern Oid GetUserMappingId(Oid userid, Oid serverid); +extern Oid GetUserMappingId(Oid userid, Oid serverid, bool missing_ok); extern UserMapping *GetUserMappingById(Oid umid); extern ForeignDataWrapper *GetForeignDataWrapper(Oid fdwid); extern ForeignDataWrapper *GetForeignDataWrapperByName(const char *name,