Обсуждение: Re: [COMMITTERS] 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

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

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

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

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

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

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

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

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

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

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

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