pgsql: Only try to push down foreign joins if the user mapping OIDs mat

Hi,

On 2016-01-28 19:09:01 +0000, 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.

Andres

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

On Fri, Mar 11, 2016 at 10:15 PM, Andres Freund <andres@anarazel.de> wrote:

On 2016-01-28 19:09:01 +0000, 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.

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

Hi,

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

On Fri, Mar 11, 2016 at 10:15 PM, Andres Freund <andres@anarazel.de> wrote:

On 2016-01-28 19:09:01 +0000, 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.

- Andres

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

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 <andres@anarazel.de> wrote:

On 2016-01-28 19:09:01 +0000, 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

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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

On 2016/03/14 11:51, Tom Lane wrote:

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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

On Mon, Mar 14, 2016 at 8:21 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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

On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> 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

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane <tgl@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?

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

On Mon, Mar 14, 2016 at 1:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane <tgl@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

Here's patch which fixes the issue using Robert's idea.

On Mon, Mar 14, 2016 at 10:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Mar 14, 2016 at 1:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane <tgl@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

On Tue, Mar 15, 2016 at 6:44 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> 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

On Wed, Mar 16, 2016 at 2:14 AM, Robert Haas <robertmhaas@gmail.com> wrote:

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

On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
ashutosh.bapat@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

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
ashutosh.bapat@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

On Wed, Mar 16, 2016 at 10:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
ashutosh.bapat@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

On Thu, Mar 17, 2016 at 2:26 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

On Wed, Mar 16, 2016 at 10:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
ashutosh.bapat@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