On Wed, Mar 23, 2016 at 12:53 PM, Robert Haas <robertmh...@gmail.com> wrote:
> On Wed, Mar 23, 2016 at 5:24 AM, Rajkumar Raghuwanshi
> <rajkumar.raghuwan...@enterprisedb.com> wrote:
>> Thanks Ashutosh for the patch. I have apply and retested it, now not getting
>> server crash.
>
> Thanks for the report and the testing. I have committed the patch.
So it's been pointed out to me off-list that I have committed a
different patch than the one I intended to commit - that is, the one
Michael sent, not the one Ashutosh sent. Oops.
Reverting Michael's patch and applying Ashutosh's doesn't work any
more due to conflicts in the regression tests. And in re-rereviewing
Ashutosh's patch I came to feel like the comments in this area needed
a lot more work.
So, barring objections, I intend to apply the attached fixup patch,
which replaces Michael's logic with Ashutosh's logic and rewrites the
comments such to be much more explicit.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From 5de87782f3fb5898edbdd870e53f08f5e83ddbe5 Mon Sep 17 00:00:00 2001
From: Robert Haas <rh...@postgresql.org>
Date: Thu, 12 May 2016 14:46:08 -0400
Subject: [PATCH] postgres_fdw: Fix the fix for crash when pushing down
multiple joins.
Commit 3151f16e1874db82ed85a005dac15368903ca9fb was intended to be
a commit of a patch from Ashutosh Bapat, but instead I mistakenly
committed an earlier version from Michael Paquier (because both
patches were submitted with the same filename, and I confused them).
Michael's patch fixes the crash but doesn't actually implement the
correct test.
Repair the incorrect logic, and also expand the comments considerably
so that this is all more clear.
Ashutosh Bapat and Robert Haas
---
contrib/postgres_fdw/expected/postgres_fdw.out | 68 ++++++++++++++++++++++++++
contrib/postgres_fdw/postgres_fdw.c | 36 ++++++++++++--
contrib/postgres_fdw/sql/postgres_fdw.sql | 10 ++++
3 files changed, 110 insertions(+), 4 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 698aa8f..1de0bc4 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -478,6 +478,74 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.
110
(10 rows)
+-- Test similar to above, except that the full join prevents any equivalence
+-- classes from being merged. This produces single relation equivalence classes
+-- included in join restrictions.
+EXPLAIN (COSTS false, VERBOSE)
+ SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+ QUERY PLAN
+------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Limit
+ Output: t1."C 1", t2.c1, t3.c1
+ -> Merge Right Join
+ Output: t1."C 1", t2.c1, t3.c1
+ Merge Cond: (t3.c1 = t1."C 1")
+ -> Foreign Scan
+ Output: t3.c1, t2.c1
+ Relations: (public.ft2 t3) LEFT JOIN (public.ft1 t2)
+ Remote SQL: SELECT r3."C 1", r2."C 1" FROM ("S 1"."T 1" r3 LEFT JOIN "S 1"."T 1" r2 ON (((r2."C 1" = r3."C 1")))) ORDER BY r3."C 1" ASC NULLS LAST
+ -> Index Only Scan using t1_pkey on "S 1"."T 1" t1
+ Output: t1."C 1"
+(11 rows)
+
+SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+ C 1 | c1 | c1
+-----+-----+-----
+ 101 | 101 | 101
+ 102 | 102 | 102
+ 103 | 103 | 103
+ 104 | 104 | 104
+ 105 | 105 | 105
+ 106 | 106 | 106
+ 107 | 107 | 107
+ 108 | 108 | 108
+ 109 | 109 | 109
+ 110 | 110 | 110
+(10 rows)
+
+-- Test similar to above with all full outer joins
+EXPLAIN (COSTS false, VERBOSE)
+ SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+ QUERY PLAN
+------------------------------------------------------------------------------------------------------------------------------------------------------------------
+ Limit
+ Output: t1."C 1", t2.c1, t3.c1
+ -> Merge Full Join
+ Output: t1."C 1", t2.c1, t3.c1
+ Merge Cond: (t3.c1 = t1."C 1")
+ -> Foreign Scan
+ Output: t2.c1, t3.c1
+ Relations: (public.ft1 t2) FULL JOIN (public.ft2 t3)
+ Remote SQL: SELECT r2."C 1", r3."C 1" FROM ("S 1"."T 1" r2 FULL JOIN "S 1"."T 1" r3 ON (((r2."C 1" = r3."C 1")))) ORDER BY r3."C 1" ASC NULLS LAST
+ -> Index Only Scan using t1_pkey on "S 1"."T 1" t1
+ Output: t1."C 1"
+(11 rows)
+
+SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+ C 1 | c1 | c1
+-----+-----+-----
+ 101 | 101 | 101
+ 102 | 102 | 102
+ 103 | 103 | 103
+ 104 | 104 | 104
+ 105 | 105 | 105
+ 106 | 106 | 106
+ 107 | 107 | 107
+ 108 | 108 | 108
+ 109 | 109 | 109
+ 110 | 110 | 110
+(10 rows)
+
RESET enable_hashjoin;
RESET enable_nestloop;
-- ===================================================================
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2f49268..4d17272 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -669,6 +669,15 @@ postgresGetForeignRelSize(PlannerInfo *root,
* we have and want to test whether any of them are useful. For a foreign
* table, we don't know what indexes are present on the remote side but
* want to speculate about which ones we'd like to use if they existed.
+ *
+ * This function returns a list of potentially-useful equivalence classes,
+ * but it does not guarantee that an EquivalenceMember exists which contains
+ * Vars only from the given relation. For example, given ft1 JOIN t1 ON
+ * ft1.x + t1.x = 0, this function will say that the equivalence class
+ * containing ft1.x + t1.x is potentially useful. Supposing ft1 is remote and
+ * t1 is local (or on a different server), it will turn out that no useful
+ * ORDER BY clause can be generated. It's not our job to figure that out
+ * here; we're only interested in identifying relevant ECs.
*/
static List *
get_useful_ecs_for_relation(PlannerInfo *root, RelOptInfo *rel)
@@ -721,13 +730,32 @@ get_useful_ecs_for_relation(PlannerInfo *root, RelOptInfo *rel)
/*
* restrictinfo->mergeopfamilies != NIL is sufficient to guarantee
* that left_ec and right_ec will be initialized, per comments in
- * distribute_qual_to_rels, and rel->joininfo should only contain ECs
- * where this relation appears on one side or the other.
+ * distribute_qual_to_rels.
+ *
+ * We want to identify which side of this merge-joinable clause
+ * contains columns from the relation produced by this RelOptInfo. We
+ * test for overlap, not containment, because there could be extra
+ * relations on either side. For example, suppose we've got something
+ * like ((A JOIN B ON A.x = B.x) JOIN C ON A.y = C.y) LEFT JOIN D ON
+ * A.y = D.y. The input rel might be the joinrel between A and B, and
+ * we'll consider the join clause A.y = D.y. relids contains a
+ * relation not involved in the join class (B) and the equivalence
+ * class for the left-hand side of the clause contains a relation not
+ * involved in the input rel (C). Despite the fact that we have only
+ * overlap and not containment in either direction, A.y is potentially
+ * useful as a sort column.
+ *
+ * Note that it's even possible that relids overlaps neither side of
+ * the join clause. For example, consider A LEFT JOIN B ON A.x = B.x
+ * AND A.x = 1. The clause A.x = 1 will appear in B's joininfo list,
+ * but overlaps neither side of B. In that case, we just skip this
+ * join clause, since it doesn't suggest a useful sort order for this
+ * relation.
*/
- if (bms_is_subset(relids, restrictinfo->right_ec->ec_relids))
+ if (bms_overlap(relids, restrictinfo->right_ec->ec_relids))
useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
restrictinfo->right_ec);
- else if (bms_is_subset(relids, restrictinfo->left_ec->ec_relids))
+ else if (bms_overlap(relids, restrictinfo->left_ec->ec_relids))
useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
restrictinfo->left_ec);
}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index d1f44d6..6c2b08c 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -242,6 +242,16 @@ SELECT t1.c1, t2."C 1" FROM ft2 t1 LEFT JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1"
EXPLAIN (COSTS false, VERBOSE)
SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+-- Test similar to above, except that the full join prevents any equivalence
+-- classes from being merged. This produces single relation equivalence classes
+-- included in join restrictions.
+EXPLAIN (COSTS false, VERBOSE)
+ SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+-- Test similar to above with all full outer joins
+EXPLAIN (COSTS false, VERBOSE)
+ SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
RESET enable_hashjoin;
RESET enable_nestloop;
--
2.5.4 (Apple Git-61)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
