On 2016/06/14 6:51, Robert Haas wrote:
> On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas <robertmh...@gmail.com> wrote:
>> Although I have done a bit of review of this patch, it needs more
>> thought than I have so far had time to give it. I will update again
>> by Tuesday.
>
> I've reviewed this a bit further and have discovered an infelicity.
> The following is all with the patch applied.
>
> By itself, this join can be pushed down:
>
> contrib_regression=# EXPLAIN SELECT 13 FROM ft1 RIGHT JOIN ft2 ON
> ft1.c1 = ft2.c1;
> QUERY PLAN
> ------------------------------------------------------
> Foreign Scan (cost=100.00..137.66 rows=822 width=4)
> Relations: (public.ft2) LEFT JOIN (public.ft1)
> (2 rows)
>
> That's great. However, when that query is used as the outer rel of a
> left join, it can't:
>
> contrib_regression=# explain verbose select * from ft4 LEFT JOIN
> (SELECT 13 FROM ft1 RIGHT JOIN ft2 ON ft1.c1 = ft2.c1) q on true;
> QUERY PLAN
> ---------------------------------------------------------------------------------------------
> Nested Loop Left Join (cost=353.50..920.77 rows=41100 width=19)
> Output: ft4.c1, ft4.c2, ft4.c3, (13)
> -> Foreign Scan on public.ft4 (cost=100.00..102.50 rows=50 width=15)
> Output: ft4.c1, ft4.c2, ft4.c3
> Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
> -> Materialize (cost=253.50..306.57 rows=822 width=4)
> Output: (13)
> -> Hash Left Join (cost=253.50..302.46 rows=822 width=4)
> Output: 13
> Hash Cond: (ft2.c1 = ft1.c1)
> -> Foreign Scan on public.ft2 (cost=100.00..137.66
> rows=822 width=4)
> Output: ft2.c1
> Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
> -> Hash (cost=141.00..141.00 rows=1000 width=4)
> Output: ft1.c1
> -> Foreign Scan on public.ft1
> (cost=100.00..141.00 rows=1000 width=4)
> Output: ft1.c1
> Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
> (18 rows)
>
> Of course, because of the PlaceHolderVar, there's no way to push down
> the ft1-ft2-ft4 join to the remote side. But we could still push down
> the ft1-ft2 join and then locally perform the join between the result
> and ft4. However, the proposed fix doesn't allow that, because
> ph_eval_at is (b 4 5) and relids for the ft1-ft2 join is also (b 4 5),
> and so the bms_is_subset(phinfo->ph_eval_at, relids) test returns
> true.
You're right. It indeed should be possible to push down ft1-ft2 join.
However it could not be done without also modifying
build_tlist_to_deparse() a little (as Ashutosh proposed [1] to do
upthread). Attached new version of the patch with following changes:
1) Modified the check in foreign_join_ok() so that it is not overly
restrictive. Previously, it used ph_needed as the highest level at which
the PHV is needed (by definition) and checked using it whether it is above
the join under consideration, which ended up being an overkill. ISTM, we
can just decide from joinrel->relids of the join under consideration
whether we are above the lowest level where the PHV could be evaluated
(ph_eval_at) and stop if so. So in the example you provided, it won't now
reject {ft1, ft2}, but will {ft4, ft1, ft2}.
2) Modified build_tlist_to_deparse() to get rid of the PlaceHolderVars in
the targetlist to deparse by using PVC_RECURSE_PLACEHOLDER flag of
pull_var_clause(). That is because we do not yet support anything other
than Vars in deparseExplicitTargetList() (+1 to your patch to change the
Assert to elog).
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/CAFjFpRfQRKNCvfPj8p%3DD9%2BDVZeuTfSN3hnGowKho%3DrKCSeD9dw%40mail.gmail.com
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 7d2512c..1abff3f 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -731,7 +731,9 @@ build_tlist_to_deparse(RelOptInfo *foreignrel)
* We require columns specified in foreignrel->reltarget->exprs and those
* required for evaluating the local conditions.
*/
- tlist = add_to_flat_tlist(tlist, foreignrel->reltarget->exprs);
+ tlist = add_to_flat_tlist(tlist,
+ pull_var_clause((Node *) foreignrel->reltarget->exprs,
+ PVC_RECURSE_PLACEHOLDERS));
tlist = add_to_flat_tlist(tlist,
pull_var_clause((Node *) fpinfo->local_conds,
PVC_RECURSE_PLACEHOLDERS));
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 1de0bc4..73900d9 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2202,6 +2202,64 @@ SELECT t1.c1, t2.c1 FROM (ft5 t1 JOIN v_ft5 t2 ON (t1.c1 = t2.c1)) left join (ft
Remote SQL: SELECT c1 FROM "S 1"."T 4"
(27 rows)
+-- non-Var items in targelist of the nullable rel of a join preventing
+-- push-down in some cases
+-- unable to push {ft1, ft2}
+EXPLAIN (COSTS false, VERBOSE)
+SELECT q.a, ft2.c1 FROM (SELECT 13 FROM ft1 WHERE c1 = 13) q(a) RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 BETWEEN 10 AND 15;
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------------------
+ Nested Loop Left Join
+ Output: (13), ft2.c1
+ Join Filter: (13 = ft2.c1)
+ -> Foreign Scan on public.ft2
+ Output: ft2.c1
+ Remote SQL: SELECT "C 1" FROM "S 1"."T 1" WHERE (("C 1" >= 10)) AND (("C 1" <= 15)) ORDER BY "C 1" ASC NULLS LAST
+ -> Materialize
+ Output: (13)
+ -> Foreign Scan on public.ft1
+ Output: 13
+ Remote SQL: SELECT NULL FROM "S 1"."T 1" WHERE (("C 1" = 13))
+(11 rows)
+
+SELECT q.a, ft2.c1 FROM (SELECT 13 FROM ft1 WHERE c1 = 13) q(a) RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 BETWEEN 10 AND 15;
+ a | c1
+----+----
+ | 10
+ | 11
+ | 12
+ 13 | 13
+ | 14
+ | 15
+(6 rows)
+
+-- ok to push {ft1, ft2} but not {ft1, ft2, ft4}
+EXPLAIN (COSTS false, VERBOSE)
+SELECT ft4.c1, q.* FROM ft4 LEFT JOIN (SELECT 13, ft1.c1, ft2.c1 FROM ft1 RIGHT JOIN ft2 ON (ft1.c1 = ft2.c1) WHERE ft1.c1 = 12) q(a, b, c) ON (ft4.c1 = q.b) WHERE ft4.c1 BETWEEN 10 AND 15;
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Nested Loop Left Join
+ Output: ft4.c1, (13), ft1.c1, ft2.c1
+ Join Filter: (ft4.c1 = ft1.c1)
+ -> Foreign Scan on public.ft4
+ Output: ft4.c1, ft4.c2, ft4.c3
+ Remote SQL: SELECT c1 FROM "S 1"."T 3" WHERE ((c1 >= 10)) AND ((c1 <= 15))
+ -> Materialize
+ Output: ft1.c1, ft2.c1, (13)
+ -> Foreign Scan
+ Output: ft1.c1, ft2.c1, 13
+ Relations: (public.ft1) INNER JOIN (public.ft2)
+ Remote SQL: SELECT r4."C 1", r5."C 1" FROM ("S 1"."T 1" r4 INNER JOIN "S 1"."T 1" r5 ON (((r5."C 1" = 12)) AND ((r4."C 1" = 12)))) ORDER BY r4."C 1" ASC NULLS LAST
+(12 rows)
+
+SELECT ft4.c1, q.* FROM ft4 LEFT JOIN (SELECT 13, ft1.c1, ft2.c1 FROM ft1 RIGHT JOIN ft2 ON (ft1.c1 = ft2.c1) WHERE ft1.c1 = 12) q(a, b, c) ON (ft4.c1 = q.b) WHERE ft4.c1 BETWEEN 10 AND 15;
+ c1 | a | b | c
+----+----+----+----
+ 10 | | |
+ 12 | 13 | 12 | 12
+ 14 | | |
+(3 rows)
+
-- recreate the dropped user mapping for further tests
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
DROP USER MAPPING FOR PUBLIC SERVER loopback;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 4e31b8e..bbd54ec 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3960,6 +3960,8 @@ postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
* 3) All join conditions are safe to push down
* 4) No relation has local filter (this can be relaxed for INNER JOIN, if we
* can move unpushable clauses upwards in the join tree).
+ * 5) It is not above the lowest level where some PlaceHolderVar in this
+ * joinrel's targetlist can be evaluated.
*/
static bool
foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
@@ -4036,6 +4038,20 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
return false;
}
+ /*
+ * Do not push down the join if it is above the lowest level where some
+ * PlaceHolderVar in this joinrel's targetlist can be evalauted.
+ */
+ foreach(lc, root->placeholder_list)
+ {
+ PlaceHolderInfo *phinfo = lfirst(lc);
+ Relids relids = joinrel->relids;
+
+ if (bms_is_subset(phinfo->ph_eval_at, relids) &&
+ bms_nonempty_difference(relids, phinfo->ph_eval_at))
+ return false;
+ }
+
/* Save the join clauses, for later use. */
fpinfo->joinclauses = joinclauses;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 6c2b08c..a2b03a1 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -527,6 +527,18 @@ EXECUTE join_stmt;
EXPLAIN (COSTS false, VERBOSE)
SELECT t1.c1, t2.c1 FROM (ft5 t1 JOIN v_ft5 t2 ON (t1.c1 = t2.c1)) left join (ft5 t3 JOIN v_ft5 t4 ON (t3.c1 = t4.c1)) ON (t1.c1 = t3.c1);
+-- non-Var items in targelist of the nullable rel of a join preventing
+-- push-down in some cases
+-- unable to push {ft1, ft2}
+EXPLAIN (COSTS false, VERBOSE)
+SELECT q.a, ft2.c1 FROM (SELECT 13 FROM ft1 WHERE c1 = 13) q(a) RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 BETWEEN 10 AND 15;
+SELECT q.a, ft2.c1 FROM (SELECT 13 FROM ft1 WHERE c1 = 13) q(a) RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 BETWEEN 10 AND 15;
+
+-- ok to push {ft1, ft2} but not {ft1, ft2, ft4}
+EXPLAIN (COSTS false, VERBOSE)
+SELECT ft4.c1, q.* FROM ft4 LEFT JOIN (SELECT 13, ft1.c1, ft2.c1 FROM ft1 RIGHT JOIN ft2 ON (ft1.c1 = ft2.c1) WHERE ft1.c1 = 12) q(a, b, c) ON (ft4.c1 = q.b) WHERE ft4.c1 BETWEEN 10 AND 15;
+SELECT ft4.c1, q.* FROM ft4 LEFT JOIN (SELECT 13, ft1.c1, ft2.c1 FROM ft1 RIGHT JOIN ft2 ON (ft1.c1 = ft2.c1) WHERE ft1.c1 = 12) q(a, b, c) ON (ft4.c1 = q.b) WHERE ft4.c1 BETWEEN 10 AND 15;
+
-- recreate the dropped user mapping for further tests
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
DROP USER MAPPING FOR PUBLIC SERVER loopback;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
