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

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