Here's patch which fixes the issue using Robert's idea.
On Mon, Mar 14, 2016 at 10:53 PM, Robert Haas <robertmh...@gmail.com> wrote:
> On Mon, Mar 14, 2016 at 1:05 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmh...@gmail.com> writes:
> >> On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> >>> I'm not really sold on enforcing that people create meaningless user
> >>> mappings. For one thing, they're likely to be sloppy about it, which
> >>> could lead to latent security problems if the FDW later acquires a
> >>> concept that user mappings mean something.
> >
> >> I think we should just fix build_simple_rel() so that it doesn't fail
> >> if there is no user mapping. It can just set rel->umid to InvalidOid
> >> in that case, and if necessary we can adjust the code elsewhere to
> >> tolerate that. This wasn't an intentional behavior change, and I
> >> think we should just put things back to the way they were.
> >
> > So, allow rel->umid to be InvalidOid if there's no user mapping, and
> > when dealing with a join, allow combining when both sides have
> InvalidOid?
>
> Exactly. And we should make sure (possibly with a regression test)
> that postgres_fdw handles that case correctly - i.e. with the right
> error.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source
index 416753d..35db4af 100644
--- a/contrib/file_fdw/input/file_fdw.source
+++ b/contrib/file_fdw/input/file_fdw.source
@@ -166,20 +166,23 @@ DROP TABLE agg;
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
SET ROLE file_fdw_user;
SELECT * FROM agg_text ORDER BY a;
SET ROLE no_priv_user;
SELECT * FROM agg_text ORDER BY a; -- ERROR
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;
ALTER FOREIGN TABLE agg_text OWNER TO file_fdw_user;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
SET ROLE file_fdw_user;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
SET ROLE file_fdw_superuser;
-- cleanup
diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source
index 8719694..26f4999 100644
--- a/contrib/file_fdw/output/file_fdw.source
+++ b/contrib/file_fdw/output/file_fdw.source
@@ -315,31 +315,41 @@ SELECT * FROM agg_text ORDER BY a; -- ERROR
ERROR: permission denied for relation agg_text
SET ROLE file_fdw_user;
\t on
EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_text WHERE a > 0;
Foreign Scan on public.agg_text
Output: a, b
Filter: (agg_text.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;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
SET ROLE file_fdw_user;
ALTER FOREIGN TABLE agg_text OPTIONS (SET format 'text');
ERROR: only superuser can change options of a file_fdw foreign table
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
drop cascades to foreign table agg_csv
drop cascades to foreign table agg_bad
drop cascades to foreign table text_csv
DROP ROLE file_fdw_superuser, file_fdw_user, no_priv_user;
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 48bdbef..48a7afa 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -1699,27 +1699,44 @@ EXECUTE join_stmt;
30 | 30
32 |
34 |
36 | 36
38 |
40 |
(10 rows)
-- 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;
-- change owner of v_ft5 to view_owner so that the effective user for scan on
-- ft5 is view_owner and not the current user.
ALTER VIEW v_ft5 OWNER TO view_owner;
-- create a public user mapping for loopback server
-- drop user mapping for current_user.
DROP USER MAPPING FOR CURRENT_USER SERVER loopback;
CREATE USER MAPPING FOR PUBLIC SERVER loopback;
@@ -1762,20 +1779,54 @@ EXPLAIN (COSTS false, VERBOSE) EXECUTE join_stmt;
-> Foreign Scan on public.ft5
Output: ft5.c1, ft5.c2, ft5.c3
Remote SQL: SELECT c1 FROM "S 1"."T 4" ORDER BY c1 ASC NULLS LAST
(13 rows)
EXECUTE join_stmt;
c1 | c1
----+----
(0 rows)
+-- Joins involving joins, which can't be pushed down, should not be pushed down
+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;
-- ===================================================================
-- parameterized queries
-- ===================================================================
-- simple join
PREPARE st1(int, int) AS SELECT t1.c3, t2.c3 FROM ft1 t1, ft2 t2 WHERE t1.c1 = $1 AND t2.c1 = $2;
EXPLAIN (VERBOSE, COSTS false) EXECUTE st1(1, 2);
QUERY PLAN
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 40bffd6..45873c3 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3361,20 +3361,30 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
JoinPathExtraData *extra)
{
PgFdwRelationInfo *fpinfo;
PgFdwRelationInfo *fpinfo_o;
PgFdwRelationInfo *fpinfo_i;
ListCell *lc;
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 not 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.
*/
if (jointype != JOIN_INNER && jointype != JOIN_LEFT &&
jointype != JOIN_RIGHT && jointype != JOIN_FULL)
return false;
/*
* If either of the joining relations is marked as unsafe to pushdown, the
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 4b88a30..8487318 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -435,25 +435,24 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM
CREATE USER view_owner;
-- grant privileges on ft4 and ft5 to view_owner
GRANT ALL ON ft4 TO view_owner;
GRANT ALL ON ft5 TO view_owner;
-- prepare statement with current session user
PREPARE join_stmt AS SELECT t1.c1, t2.c1 FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 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;
CREATE VIEW v_ft5 AS SELECT * FROM ft5;
-- change owner of v_ft5 to view_owner so that the effective user for scan on
-- ft5 is view_owner and not the current user.
ALTER VIEW v_ft5 OWNER TO view_owner;
-- create a public user mapping for loopback server
-- drop user mapping for current_user.
DROP USER MAPPING FOR CURRENT_USER SERVER loopback;
@@ -463,20 +462,24 @@ CREATE USER MAPPING FOR PUBLIC SERVER loopback;
PREPARE join_stmt AS SELECT t1.c1, t2.c1 FROM ft5 t1 JOIN v_ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 100 LIMIT 10;
EXPLAIN (COSTS false, VERBOSE) EXECUTE join_stmt;
EXECUTE join_stmt;
-- create user mapping for view_owner and execute the prepared statement
-- the join should not be pushed down since joining relations now use two
-- different user mappings
CREATE USER MAPPING FOR view_owner SERVER loopback;
EXPLAIN (COSTS false, VERBOSE) EXECUTE join_stmt;
EXECUTE join_stmt;
+-- Joins involving joins, which can't be pushed down, should not be pushed down
+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;
-- ===================================================================
-- parameterized queries
-- ===================================================================
-- simple join
PREPARE st1(int, int) AS SELECT t1.c3, t2.c3 FROM ft1 t1, ft2 t2 WHERE t1.c1 = $1 AND t2.c1 = $2;
EXPLAIN (VERBOSE, COSTS false) EXECUTE st1(1, 2);
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index 239849b..f1feb85 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -24,21 +24,21 @@
#include "miscadmin.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
#include "utils/rel.h"
#include "utils/syscache.h"
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.
*/
ForeignDataWrapper *
GetForeignDataWrapper(Oid fdwid)
{
Form_pg_foreign_data_wrapper fdwform;
ForeignDataWrapper *fdw;
Datum datum;
@@ -216,21 +216,21 @@ GetUserMappingById(Oid umid)
* PUBLIC mappings (userid == InvalidOid).
*/
UserMapping *
GetUserMapping(Oid userid, Oid serverid)
{
Datum datum;
HeapTuple tp;
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);
um->userid = userid;
um->serverid = serverid;
/* Extract the umoptions */
datum = SysCacheGetAttr(USERMAPPINGUSERSERVER,
tp,
Anum_pg_user_mapping_umoptions,
@@ -243,66 +243,84 @@ GetUserMapping(Oid userid, Oid serverid)
ReleaseSysCache(tp);
return um;
}
/*
* GetUserMappingId - look up the user mapping, and return its OID
*
* 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);
ReleaseSysCache(tp);
return umid;
}
/*
* find_user_mapping - Guts of GetUserMapping family.
*
* 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;
tp = SearchSysCache2(USERMAPPINGUSERSERVER,
ObjectIdGetDatum(userid),
ObjectIdGetDatum(serverid));
if (HeapTupleIsValid(tp))
return tp;
/* Not found for the specific user -- try PUBLIC */
tp = SearchSysCache2(USERMAPPINGUSERSERVER,
ObjectIdGetDatum(InvalidOid),
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;
}
/*
* GetForeignTable - look up the foreign table definition by relation oid.
*/
ForeignTable *
GetForeignTable(Oid relid)
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index 763d39d..aea69b0 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -176,25 +176,30 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptKind reloptkind)
if (rte->relkind == RELKIND_FOREIGN_TABLE)
{
/*
* This should match what ExecCheckRTEPerms() does.
*
* Note that if the plan ends up depending on the user OID in any
* way - e.g. if it depends on the computed user mapping OID - we must
* 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.
+ *
+ * An FDW may choose not to enforce user mapping, in which case there
+ * may not be a valid user mapping available for the given foreign
+ * relation. In such a case rel->umid is set to InvalidOid and
+ * rel->serverid will be valid.
*/
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;
/* Save the finished struct in the query's simple_rel_array */
root->simple_rel_array[relid] = rel;
/*
* If this rel is an appendrel parent, recurse to build "other rel"
* RelOptInfos for its children. They are "other rels" because they are
@@ -435,26 +440,30 @@ build_join_rel(PlannerInfo *root,
joinrel->joininfo = NIL;
joinrel->has_eclass_joins = false;
/*
* Set up foreign-join fields if outer and inner relation are foreign
* tables (or joins) belonging to the same server and using the same
* user mapping.
*
* 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, user mapping id
+ * associated with the joining relation may be invalid. A valid serverid
+ * differntiates between a pushded down join with invalid 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;
}
/*
* Create a new tlist containing just the vars that need to be output from
* this join (ie, are needed for higher joinclauses or final output).
*
* NOTE: the tlist order for a join rel will depend on which pair of outer
diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h
index 71f8e55..fb945e9 100644
--- a/src/include/foreign/foreign.h
+++ b/src/include/foreign/foreign.h
@@ -65,21 +65,21 @@ typedef struct ForeignTable
{
Oid relid; /* relation Oid */
Oid serverid; /* server Oid */
List *options; /* ftoptions as DefElem list */
} 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,
bool missing_ok);
extern ForeignTable *GetForeignTable(Oid relid);
extern List *GetForeignColumnOptions(Oid relid, AttrNumber attnum);
extern Oid get_foreign_data_wrapper_oid(const char *fdwname, bool missing_ok);
extern Oid get_foreign_server_oid(const char *servername, bool missing_ok);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
