Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-28 Thread Robert Haas
On Thu, Mar 17, 2016 at 2:26 AM, Ashutosh Bapat
 wrote:
> On Wed, Mar 16, 2016 at 10:22 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>> > On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
>> > ashutosh.ba...@enterprisedb.com> wrote:
>> >> 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.
>>
>> > If a query against a single table with no user mapping is legal, I don't
>> > see why pushing down a join between two tables neither of which has a
>> > user
>> > mapping shouldn't also be legal.
>>
>> The key point here is that Ashutosh is arguing on the basis of the
>> behavior of postgres_fdw, which is not representative of all FDWs.
>> The core code has no business assuming that all FDWs require user
>> mappings; file_fdw is a counterexample.
>>
>
> Here's what the patch is doing.
>
> The "core" code FDW is given chance to add paths for a join between foreign
> relations if they are from the same (valid) server and have same user
> mappings, even if the user mapping is invalid. This is code in
> build_join_rel(). So, from core code's perspective it's perfectly fine to
> push a join down when both sides do not have valid user mapping associated
> with them. So, file_fdw is capable of implementing join pushdown even
> without having user mapping.
>
> postgres_fdw code however is different. Rest of the discussion only pertains
> to postgres_fdw. The comment in postgres_fdw.sql testcase, to which Robert
> objected, is relevant only for postgres_fdw.
>
> postgres_fdw requires user mapping to execute the query. For base foreign
> tables, it's fine not to have a user mapping at the time of planning as long
> as planner doesn't need to query the foreign server. But it certainly
> requires a user mapping at the time of execution, or else it throws error at
> the time of execution. So, Robert's statement "If a query against a single
> table with no user mapping is legal" is not entirely correct in the context
> of postgres_fdw; postgresGetForeignRelSize(), which is called during
> planning, can throw error if it doesn't find a valid user mapping. If a join
> between two postgres_fdw foreign relations without valid user mappings is
> decided to be pushed down at the time of planning, it will require a user
> mapping at the time of execution. This user mapping is derived from the
> joining relations recursively. If all of them have same user mapping (even
> if invalid) it's fine. If it's invalid, we will throw error at the time of
> execution. But if they acquire different user mappings, postgres_fdw can not
> fire the join query. It can not use any of the user mappings since that will
> compromise security. So, we have landed with a plan which can not be
> executed. To be on the safer side, postgres_fdw does not push a join down if
> joining sides do not have a valid user mapping. A base foreign relation with
> postgres_fdw will require a user mapping, for all serious uses. So, it's not
> likely that we will end up with joins between postgres_fdw foreign relations
> with invalid user mapping.

Makes sense to me.  Committed.

-- 
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


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-19 Thread Ashutosh Bapat
On Wed, Mar 16, 2016 at 10:22 PM, Tom Lane  wrote:

> Robert Haas  writes:
> > On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
> > ashutosh.ba...@enterprisedb.com> wrote:
> >> 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.
>
> > If a query against a single table with no user mapping is legal, I don't
> > see why pushing down a join between two tables neither of which has a
> user
> > mapping shouldn't also be legal.
>
> The key point here is that Ashutosh is arguing on the basis of the
> behavior of postgres_fdw, which is not representative of all FDWs.
> The core code has no business assuming that all FDWs require user
> mappings; file_fdw is a counterexample.
>
>
Here's what the patch is doing.

The "core" code FDW is given chance to add paths for a join between foreign
relations if they are from the same (valid) server and have same user
mappings, even if the user mapping is invalid. This is code in
build_join_rel(). So, from core code's perspective it's perfectly fine to
push a join down when both sides do not have valid user mapping associated
with them. So, file_fdw is capable of implementing join pushdown even
without having user mapping.

postgres_fdw code however is different. Rest of the discussion only
pertains to postgres_fdw. The comment in postgres_fdw.sql testcase, to
which Robert objected, is relevant only for postgres_fdw.

postgres_fdw requires user mapping to execute the query. For base foreign
tables, it's fine not to have a user mapping at the time of planning as
long as planner doesn't need to query the foreign server. But it certainly
requires a user mapping at the time of execution, or else it throws error
at the time of execution. So, Robert's statement "If a query against a
single table with no user mapping is legal" is not entirely correct in the
context of postgres_fdw; postgresGetForeignRelSize(), which is called
during planning, can throw error if it doesn't find a valid user mapping.
If a join between two postgres_fdw foreign relations without valid user
mappings is decided to be pushed down at the time of planning, it will
require a user mapping at the time of execution. This user mapping is
derived from the joining relations recursively. If all of them have same
user mapping (even if invalid) it's fine. If it's invalid, we will throw
error at the time of execution. But if they acquire different user
mappings, postgres_fdw can not fire the join query. It can not use any of
the user mappings since that will compromise security. So, we have landed
with a plan which can not be executed. To be on the safer side,
postgres_fdw does not push a join down if joining sides do not have a valid
user mapping. A base foreign relation with postgres_fdw will require a user
mapping, for all serious uses. So, it's not likely that we will end up with
joins between postgres_fdw foreign relations with invalid user mapping.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-19 Thread Robert Haas
On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
> 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.
>

If a query against a single table with no user mapping is legal, I don't
see why pushing down a join between two tables neither of which has a user
mapping shouldn't also be legal.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-19 Thread Tom Lane
Robert Haas  writes:
> On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
> ashutosh.ba...@enterprisedb.com> wrote:
>> 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.

> If a query against a single table with no user mapping is legal, I don't
> see why pushing down a join between two tables neither of which has a user
> mapping shouldn't also be legal.

The key point here is that Ashutosh is arguing on the basis of the
behavior of postgres_fdw, which is not representative of all FDWs.
The core code has no business assuming that all FDWs require user
mappings; file_fdw is a counterexample.

I think the behavior Robert suggests is just fine.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-16 Thread Ashutosh Bapat
On Wed, Mar 16, 2016 at 2:14 AM, Robert Haas  wrote:

> On Tue, Mar 15, 2016 at 6:44 AM, Ashutosh Bapat
>  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 

Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-15 Thread Robert Haas
On Tue, Mar 15, 2016 at 6:44 AM, Ashutosh Bapat
 wrote:
> Here's patch which fixes the issue using Robert's idea.

Please at least check your patches with 'git diff --check' 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.

 -- 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.

-- 
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


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-15 Thread Ashutosh Bapat
Here's patch which fixes the issue using Robert's idea.

On Mon, Mar 14, 2016 at 10:53 PM, Robert Haas  wrote:

> On Mon, Mar 14, 2016 at 1:05 PM, Tom Lane  wrote:
> > Robert Haas  writes:
> >> On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane  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 

Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-14 Thread Robert Haas
On Mon, Mar 14, 2016 at 1:05 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane  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


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-14 Thread Tom Lane
Robert Haas  writes:
> On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane  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?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-14 Thread Robert Haas
On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane  wrote:
> Etsuro Fujita  writes:
>> On 2016/03/13 4:46, Andres Freund wrote:
>>> ... The difference apears to be the
>>> check that's now in build_simple_rel() - there was nothing hitting the
>>> user mapping code before for file_fdw.
>
>> Exactly.
>
>> I'm not sure it's worth complicating the code to keep that behavior, so
>> I'd vote for adding the change notice to 9.6 release notes or something
>> like that in addition to updating file-fdw.sgml.
>
> Perhaps it would be useful for an FDW to be able to specify that user
> mappings are meaningless to it?  And then bypass this check for such FDWs?
>
> 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.

-- 
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


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-14 Thread Ashutosh Bapat
On Mon, Mar 14, 2016 at 8:21 AM, Tom Lane  wrote:

> Etsuro Fujita  writes:
> > On 2016/03/13 4:46, Andres Freund wrote:
> >> ... The difference apears to be the
> >> check that's now in build_simple_rel() - there was nothing hitting the
> >> user mapping code before for file_fdw.
>
> > Exactly.
>
> > I'm not sure it's worth complicating the code to keep that behavior, so
> > I'd vote for adding the change notice to 9.6 release notes or something
> > like that in addition to updating file-fdw.sgml.
>
> Perhaps it would be useful for an FDW to be able to specify that user
> mappings are meaningless to it?  And then bypass this check for such FDWs?
>

In such a case, whether FDWs be given chance to push down joins? I guess
the answer is yes, but wanted to confirm.


>
> 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.
>
>
Hmm. Should we let FDW handler set a boolean in fdwroutine specifying
whether the core code should bother about user mapping or not. This way the
author of FDW decides whether s/he is going to write code that uses user
mappings or not. A small problem with that is PG documentation describes
fdwroutine as a structure holding function pointers and now it will store a
boolean variable. But I think that can be managed either by having this
option as a function pointer returning boolean or changing the
documentation.

Other problem is what should we do when a user creates or has an existing
user mapping for an FDW which specifies that user mapping is meaningless to
it. Should we allow the user mapping to be created but ignore it or do not
allow it to be created? In the later case, what should happen to the
existing user mappings?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-13 Thread Etsuro Fujita
On 2016/03/14 11:51, Tom Lane wrote:
> Etsuro Fujita  writes:
>> On 2016/03/13 4:46, Andres Freund wrote:
>>> ... The difference apears to be the
>>> check that's now in build_simple_rel() - there was nothing hitting the
>>> user mapping code before for file_fdw.

>> Exactly.

>> I'm not sure it's worth complicating the code to keep that behavior, so
>> I'd vote for adding the change notice to 9.6 release notes or something
>> like that in addition to updating file-fdw.sgml.

> Perhaps it would be useful for an FDW to be able to specify that user
> mappings are meaningless to it?  And then bypass this check for such FDWs?
> 
> 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.

Seems reasonable.

Best regards,
Etsuro Fujita




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-13 Thread Tom Lane
Etsuro Fujita  writes:
> On 2016/03/13 4:46, Andres Freund wrote:
>> ... The difference apears to be the
>> check that's now in build_simple_rel() - there was nothing hitting the
>> user mapping code before for file_fdw.

> Exactly.

> I'm not sure it's worth complicating the code to keep that behavior, so 
> I'd vote for adding the change notice to 9.6 release notes or something 
> like that in addition to updating file-fdw.sgml.

Perhaps it would be useful for an FDW to be able to specify that user
mappings are meaningless to it?  And then bypass this check for such FDWs?

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.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

2016-03-13 Thread Etsuro Fujita

Hi,

On 2016/03/13 4:46, Andres Freund wrote:

On 2016-03-12 11:56:24 -0500, Robert Haas wrote:

On Fri, Mar 11, 2016 at 10:15 PM, Andres Freund  wrote:

On 2016-01-28 19:09:01 +, Robert Haas wrote:

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.



I noticed that this breaks some citus regression tests in a minor
manner. Namely previously file_fdw worked without a user mapping, now it
doesn't appear to anymore.

This is easy enough to fix, and it's perfectly ok for us to fix this,
but I do wonder if that's not going to cause trouble for others.



Hmm, I didn't intend to change that.  If that commit broke something,
there's obviously a hole in our regression test coverage.



CREATE EXTENSION file_fdw;
CREATE SERVER file_server FOREIGN DATA WRAPPER file_fdw;

CREATE FOREIGN TABLE agg_csv (
 a   int2,
 b   float4
) SERVER file_server
OPTIONS (format 'csv', filename 
'/home/andres/src/postgresql/contrib/file_fdw/data/agg.csv', header 'true', 
delimiter ';', quote '@', escape '"', null '');

SELECT * FROM agg_csv;



worked in 9.5, but doesn't in master. The difference apears to be the
check that's now in build_simple_rel() - there was nothing hitting the
user mapping code before for file_fdw.


Exactly.

I'm not sure it's worth complicating the code to keep that behavior, so 
I'd vote for adding the change notice to 9.6 release notes or something 
like that in addition to updating file-fdw.sgml.


Best regards,
Etsuro Fujita




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers