Unclear regression test for postgres_fdw

Started by Antonin Houskaabout 8 years ago5 messages

ah@cybertec.at

about 8 years ago

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

Jeevan Chalke

jeevan.chalke@enterprisedb.com

about 8 years ago

In reply to: Antonin Houska (#1)

Re: Unclear regression test for postgres_fdw

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

Jeevan Chalke

jeevan.chalke@enterprisedb.com

about 8 years ago

In reply to: Jeevan Chalke (#2)

1 attachment(s)

Re: Unclear regression test for postgres_fdw

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

Attachments:

update_agg_push_down_test.patchtext/x-patch; charset=US-ASCII; name=update_agg_push_down_test.patchDownload

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 1063d92..bce3348 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -3160,21 +3160,23 @@ drop operator public.<^(int, int);
 -- 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);
-                                                QUERY PLAN                                                
-----------------------------------------------------------------------------------------------------------
+select count(t1.c3) from ft2 t1 left join ft2 t2 on (t1.c1 = random() * t2.c2);
+                                        QUERY PLAN                                         
+-------------------------------------------------------------------------------------------
  Aggregate
    Output: count(t1.c3)
-   ->  Nested Loop
+   ->  Nested Loop Left Join
          Output: t1.c3
-         ->  Foreign Scan on public.ft1 t2
-               Remote SQL: SELECT NULL FROM "S 1"."T 1"
+         Join Filter: ((t1.c1)::double precision = (random() * (t2.c2)::double precision))
+         ->  Foreign Scan on public.ft2 t1
+               Output: t1.c3, t1.c1
+               Remote SQL: SELECT "C 1", c3 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)))
-(11 rows)
+               Output: t2.c2
+               ->  Foreign Scan on public.ft2 t2
+                     Output: t2.c2
+                     Remote SQL: SELECT c2 FROM "S 1"."T 1"
+(13 rows)
 
 -- Subquery in FROM clause having aggregate
 explain (verbose, costs off)
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 0986957..1df1e3a 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -829,7 +829,7 @@ drop operator public.<^(int, int);
 -- 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);
+select count(t1.c3) from ft2 t1 left join ft2 t2 on (t1.c1 = random() * t2.c2);
 
 -- Subquery in FROM clause having aggregate
 explain (verbose, costs off)

Antonin Houska

ah@cybertec.at

about 8 years ago

In reply to: Jeevan Chalke (#3)

Re: Unclear regression test for postgres_fdw

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

Robert Haas

robertmhaas@gmail.com

about 8 years ago

In reply to: Antonin Houska (#4)

Re: Unclear regression test for postgres_fdw

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