pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedUnclear regression test for postgres_fdw
5+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2017-12-01 9502227805 postgres_fdw: Fix test that didn't test what it claimed.
* Unclear regression test for postgres_fdw
@ 2017-11-29 20:06 Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 5+ messages in thread
From: Antonin Houska @ 2017-11-29 20:06 UTC (permalink / raw)
To: pgsql-hackers
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Unclear regression test for postgres_fdw
@ 2017-11-30 10:14 Jeevan Chalke <jeevan.chalke@enterprisedb.com>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 1 reply; 5+ messages in thread
From: Jeevan Chalke @ 2017-11-30 10:14 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Unclear regression test for postgres_fdw
@ 2017-11-30 10:36 Jeevan Chalke <jeevan.chalke@enterprisedb.com>
parent: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
0 siblings, 1 reply; 5+ messages in thread
From: Jeevan Chalke @ 2017-11-30 10:36 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: pgsql-hackers
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.patch (text/x-patch, 2.6K) download
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)
* Re: Unclear regression test for postgres_fdw
@ 2017-12-01 09:01 Antonin Houska <ah@cybertec.at>
parent: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
0 siblings, 1 reply; 5+ messages in thread
From: Antonin Houska @ 2017-12-01 09:01 UTC (permalink / raw)
To: Jeevan Chalke <jeevan.chalke@enterprisedb.com>; +Cc: pgsql-hackers
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Unclear regression test for postgres_fdw
@ 2017-12-01 18:50 Robert Haas <robertmhaas@gmail.com>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 0 replies; 5+ messages in thread
From: Robert Haas @ 2017-12-01 18:50 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: Jeevan Chalke <jeevan.chalke@enterprisedb.com>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2017-12-01 18:50 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-11-29 20:06 Unclear regression test for postgres_fdw Antonin Houska <ah@cybertec.at>
2017-11-30 10:14 ` Jeevan Chalke <jeevan.chalke@enterprisedb.com>
2017-11-30 10:36 ` Jeevan Chalke <jeevan.chalke@enterprisedb.com>
2017-12-01 09:01 ` Antonin Houska <ah@cybertec.at>
2017-12-01 18:50 ` Robert Haas <robertmhaas@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox