Horton

Thread

Commits

  1. postgres_fdw: Fix test that didn't test what it claimed.

  1. Unclear regression test for postgres_fdw

    Antonin Houska <ah@cybertec.at> — 2017-11-29T20:06:11Z

    The following test

-- Input relation to aggregate push down hook is not safe to pushdown and thus
-- the aggregate cannot be pushed down to foreign server.
explain (verbose, costs off)
select count(t1.c3) from ft1 t1, ft1 t2 where t1.c1 = postgres_fdw_abs(t1.c2);

produces the following plan

                                                QUERY PLAN                                                
----------------------------------------------------------------------------------------------------------
 Aggregate
   Output: count(t1.c3)
   ->  Nested Loop
         Output: t1.c3
         ->  Foreign Scan on public.ft1 t2
               Remote SQL: SELECT NULL FROM "S 1"."T 1"
         ->  Materialize
               Output: t1.c3
               ->  Foreign Scan on public.ft1 t1
                     Output: t1.c3
                     Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1" = public.postgres_fdw_abs(c2)))

which is not major problem as such, but gdb shows that the comment "aggregate
cannot be pushed" is not correct. In fact, postgresGetForeignUpperPaths()
*does* create the upper path.

The reason that UPPERREL_GROUP_AGG is eventually not used seems to be that
postgresGetForeignJoinPaths() -> add_foreign_grouping_paths() ->
estimate_path_cost_size() estimates the join cost in rather generic way. While
the remote server can push the join clause down to the inner relation of NL,
the postgres_fdw cost computation assumes that the join clause is applied to
each pair of output and input tuple.

I don't think that the postgres_fdw's estimate can be fixed easily, but if the
impact of "shipability" on (not) using the upper relation should be tested, we
need a different test.

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

  2. Re: Unclear regression test for postgres_fdw

    Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2017-11-30T10:14:05Z

    On Thu, Nov 30, 2017 at 1:36 AM, Antonin Houska <ah@cybertec.at> wrote:

> The following test
>
> -- Input relation to aggregate push down hook is not safe to pushdown and
> thus
> -- the aggregate cannot be pushed down to foreign server.
> explain (verbose, costs off)
> select count(t1.c3) from ft1 t1, ft1 t2 where t1.c1 =
> postgres_fdw_abs(t1.c2);
>
> produces the following plan
>
>                                                 QUERY PLAN
> ------------------------------------------------------------
> ----------------------------------------------
>  Aggregate
>    Output: count(t1.c3)
>    ->  Nested Loop
>          Output: t1.c3
>          ->  Foreign Scan on public.ft1 t2
>                Remote SQL: SELECT NULL FROM "S 1"."T 1"
>          ->  Materialize
>                Output: t1.c3
>                ->  Foreign Scan on public.ft1 t1
>                      Output: t1.c3
>                      Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1"
> = public.postgres_fdw_abs(c2)))
>
> which is not major problem as such, but gdb shows that the comment
> "aggregate
> cannot be pushed" is not correct. In fact, postgresGetForeignUpperPaths()
> *does* create the upper path.
>
> The reason that UPPERREL_GROUP_AGG is eventually not used seems to be that
> postgresGetForeignJoinPaths() -> add_foreign_grouping_paths() ->
> estimate_path_cost_size() estimates the join cost in rather generic way.
> While
> the remote server can push the join clause down to the inner relation of
> NL,
> the postgres_fdw cost computation assumes that the join clause is applied
> to
> each pair of output and input tuple.
>
> I don't think that the postgres_fdw's estimate can be fixed easily, but if
> the
> impact of "shipability" on (not) using the upper relation should be
> tested, we
> need a different test.
>

Oops. My bad.
Agree with your analysis.
Will send a patch fixing this testcase.

Thank you Antonin for catching and reporting it.


>
> --
> Antonin Houska
> Cybertec Schönig & Schönig GmbH
> Gröhrmühlgasse 26
> A-2700 Wiener Neustadt
> Web: http://www.postgresql-support.de, http://www.cybertec.at
>
>


-- 
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

  3. Re: Unclear regression test for postgres_fdw

    Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2017-11-30T10:36:40Z

    On Thu, Nov 30, 2017 at 3:44 PM, Jeevan Chalke <
jeevan.chalke@enterprisedb.com> wrote:

>
>
> On Thu, Nov 30, 2017 at 1:36 AM, Antonin Houska <ah@cybertec.at> wrote:
>
>> The following test
>>
>> -- Input relation to aggregate push down hook is not safe to pushdown and
>> thus
>> -- the aggregate cannot be pushed down to foreign server.
>> explain (verbose, costs off)
>> select count(t1.c3) from ft1 t1, ft1 t2 where t1.c1 =
>> postgres_fdw_abs(t1.c2);
>>
>> produces the following plan
>>
>>                                                 QUERY PLAN
>> ------------------------------------------------------------
>> ----------------------------------------------
>>  Aggregate
>>    Output: count(t1.c3)
>>    ->  Nested Loop
>>          Output: t1.c3
>>          ->  Foreign Scan on public.ft1 t2
>>                Remote SQL: SELECT NULL FROM "S 1"."T 1"
>>          ->  Materialize
>>                Output: t1.c3
>>                ->  Foreign Scan on public.ft1 t1
>>                      Output: t1.c3
>>                      Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1"
>> = public.postgres_fdw_abs(c2)))
>>
>> which is not major problem as such, but gdb shows that the comment
>> "aggregate
>> cannot be pushed" is not correct. In fact, postgresGetForeignUpperPaths()
>> *does* create the upper path.
>>
>> The reason that UPPERREL_GROUP_AGG is eventually not used seems to be that
>> postgresGetForeignJoinPaths() -> add_foreign_grouping_paths() ->
>> estimate_path_cost_size() estimates the join cost in rather generic way.
>> While
>> the remote server can push the join clause down to the inner relation of
>> NL,
>> the postgres_fdw cost computation assumes that the join clause is applied
>> to
>> each pair of output and input tuple.
>>
>> I don't think that the postgres_fdw's estimate can be fixed easily, but
>> if the
>> impact of "shipability" on (not) using the upper relation should be
>> tested, we
>> need a different test.
>>
>
> Oops. My bad.
> Agree with your analysis.
> Will send a patch fixing this testcase.
>

Attached patch to fix the test case. In new test case I am using a JOIN
query where JOIN condition is not safe to push down and hence the JOIN
itself is unsafe. Due to which AggPushDown does not consider that relation.
Also, I have used ft2 in the query which has use_remote_estimate set to
true.

Thanks


>
>
> Thank you Antonin for catching and reporting it.
>
>
>>
>> --
>> Antonin Houska
>> Cybertec Schönig & Schönig GmbH
>> Gröhrmühlgasse 26
>> A-2700 Wiener Neustadt
>> Web: http://www.postgresql-support.de, http://www.cybertec.at
>>
>>

-- 
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

  4. Re: Unclear regression test for postgres_fdw

    Antonin Houska <ah@cybertec.at> — 2017-12-01T09:01:34Z

    Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

> On Thu, Nov 30, 2017 at 3:44 PM, Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:
> 
>  On Thu, Nov 30, 2017 at 1:36 AM, Antonin Houska <ah@cybertec.at> wrote:
> 
>  The following test
> 
>  -- Input relation to aggregate push down hook is not safe to pushdown and thus
>  -- the aggregate cannot be pushed down to foreign server.
>  explain (verbose, costs off)
>  select count(t1.c3) from ft1 t1, ft1 t2 where t1.c1 = postgres_fdw_abs(t1.c2);
> 
>  produces the following plan
> 
>  QUERY PLAN
>  ----------------------------------------------------------------------------------------------------------
>  Aggregate
>  Output: count(t1.c3)
>  -> Nested Loop
>  Output: t1.c3
>  -> Foreign Scan on public.ft1 t2
>  Remote SQL: SELECT NULL FROM "S 1"."T 1"
>  -> Materialize
>  Output: t1.c3
>  -> Foreign Scan on public.ft1 t1
>  Output: t1.c3
>  Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE (("C 1" = public.postgres_fdw_abs(c2)))
> 
>  which is not major problem as such, but gdb shows that the comment "aggregate
>  cannot be pushed" is not correct. In fact, postgresGetForeignUpperPaths()
>  *does* create the upper path.
> 
>  The reason that UPPERREL_GROUP_AGG is eventually not used seems to be that
>  postgresGetForeignJoinPaths() -> add_foreign_grouping_paths() ->
>  estimate_path_cost_size() estimates the join cost in rather generic way. While
>  the remote server can push the join clause down to the inner relation of NL,
>  the postgres_fdw cost computation assumes that the join clause is applied to
>  each pair of output and input tuple.
> 
>  I don't think that the postgres_fdw's estimate can be fixed easily, but if the
>  impact of "shipability" on (not) using the upper relation should be tested, we
>  need a different test.
> 
>  Oops. My bad.
>  Agree with your analysis.
>  Will send a patch fixing this testcase.
> 
> Attached patch to fix the test case. In new test case I am using a JOIN
> query where JOIN condition is not safe to push down and hence the JOIN
> itself is unsafe.

I've just verified that postgresGetForeignUpperPaths() does return here:

	/*
	 * If input rel is not safe to pushdown, then simply return as we cannot
	 * perform any post-join operations on the foreign server.
	 */
	if (!input_rel->fdw_private ||
		!((PgFdwRelationInfo *) input_rel->fdw_private)->pushdown_safe)
		return;

I see no other problems here. You may need to add the patch to the next CF so
it does not get lost.

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

  5. Re: Unclear regression test for postgres_fdw

    Robert Haas <robertmhaas@gmail.com> — 2017-12-01T18:50:19Z

    On Fri, Dec 1, 2017 at 4:01 AM, Antonin Houska <ah@cybertec.at> wrote:
> I see no other problems here.

Committed, thanks for the report and review.

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