On Wed, Mar 16, 2016 at 2:14 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Tue, Mar 15, 2016 at 6:44 AM, Ashutosh Bapat
> <ashutosh.ba...@enterprisedb.com> wrote:
> > Here's patch which fixes the issue using Robert's idea.
>
> Please at least check your patches with 'git diff --check'
Thanks.
> before
> submitting them. And where it's not too much trouble, pgindent them.
> Or at least make them look something like what pgindent would have
> produced, instead of having the line lengths be all over the place.
>
Sorry. PFA the patch with git diff --check output fixed.
>
> -- 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.
>
> Now what's going on here? It seems to me that either postgres_fdw
> requires a user mapping (in which case this ought to fail) or it
> doesn't (in which case this ought to push the join down). I don't
> understand how working but not pushing the join down can ever be the
> right behavior.
>
>
In 9.5, postgres_fdw allowed to prepare statements involving foreign tables
without an associated user mapping as long as planning did not require the
user mapping. Remember, planner would require user mapping in case
use_remote_estimate is ON for that foreign table. The user mapping would be
certainly required at the time of execution. So executing such a prepared
statement would throw error. If somebody created a user mapping between
prepare and execute, execute would not throw an error. A join can be pushed
down only when user mappings associated with the joining relations are
known and they match. But given the behavior of 9.5 we should let the
prepare succeed, even if the join can not be pushed down because of unknown
user mapping. Before this fix, the test was not letting the prepare
succeed, which is not compliant with 9.5.
--
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
