Thanks Michael for looking into this.


> In get_useful_ecs_for_relation, it seems to me that this assertion
> should be removed and replaces by an actual check because even if
> right_ec and left_ec are initialized, we cannot be sure that ec_relids
> contains the relations specified:
>         /*
>          * 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.
>          */
>         if (bms_is_subset(relids, restrictinfo->right_ec->ec_relids))
>             useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
>
>  restrictinfo->right_ec);
>         else
>         {
>             Assert(bms_is_subset(relids,
> restrictinfo->left_ec->ec_relids));
>             useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
>
> restrictinfo->left_ec);
>         }
>

An EC covers all the relations covered by all the equivalence members it
contains. In case of mergejoinable clause for outer join, EC may cover just
a single relation whose column appears on either side of the clause. In
this case, bms_is_subset() for a given join relation covering single
relation in EC will be false. So, we have to use bms_overlap() instead of
bms_is_subset(). The caller get_useful_pathkeys_for_rel() extracts the
equivalence member (if any), which is entirely covered by the given
relation. Otherwise, you are correct that we have to convert the assertion
into a condition. I have added comments in get_useful_ecs_for_relation()
explaining, why.

See for example the attached (with more tests including combinations
> of joins, and three-table joins). I have added an open item for 9.6 on
> the wiki.
>

Thanks for those tests. Actually, that code is relevant for joins which can
not be pushed down to the foreign server. For such joins we try to add
pathkeys which will help merge joins. I have included the relevant tests
rewriting them to use local tables, so that the entire join is not pushed
down to the foreign server.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index a7f32f3..d38ff86 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -471,20 +471,88 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.
  103
  104
  105
  106
  107
  108
  109
  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;
 -- ===================================================================
 -- WHERE with remotely-executable conditions
 -- ===================================================================
 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = 1;         -- Var, OpExpr(b), Const
                                          QUERY PLAN                                          
 ---------------------------------------------------------------------------------------------
  Foreign Scan on public.ft1 t1
    Output: c1, c2, c3, c4, c5, c6, c7, c8
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index d6db834..2c566da 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -662,20 +662,23 @@ postgresGetForeignRelSize(PlannerInfo *root,
 /*
  * get_useful_ecs_for_relation
  *		Determine which EquivalenceClasses might be involved in useful
  *		orderings of this relation.
  *
  * This function is in some respects a mirror image of the core function
  * pathkeys_useful_for_merging: for a regular table, we know what indexes
  * 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 does not check existence of an equivalence class member, which
+ * entirely belongs to the given relation.
  */
 static List *
 get_useful_ecs_for_relation(PlannerInfo *root, RelOptInfo *rel)
 {
 	List	   *useful_eclass_list = NIL;
 	ListCell   *lc;
 	Relids		relids;
 
 	/*
 	 * First, consider whether any active EC is potentially useful for a merge
@@ -714,32 +717,33 @@ get_useful_ecs_for_relation(PlannerInfo *root, RelOptInfo *rel)
 		/* Consider only mergejoinable clauses */
 		if (restrictinfo->mergeopfamilies == NIL)
 			continue;
 
 		/* Make sure we've got canonical ECs. */
 		update_mergeclause_eclasses(root, restrictinfo);
 
 		/*
 		 * 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. For outer joins, rel->joininfo may contain
+		 * clauses, which do not necessarily contain any variables from that
+		 * relation but are placed there because it's part of the outer join
+		 * where they need to be evaluated. See distribute_qual_to_rels() and
+		 * reconsider_outer_join_clauses() for details. Choose the EC whose
+		 * ec_relids overlaps with the relids covered by the given 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
-		{
-			Assert(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);
-		}
 	}
 
 	return useful_eclass_list;
 }
 
 /*
  * get_useful_pathkeys_for_relation
  *		Determine which orderings of a relation might be useful.
  *
  * Getting data in sorted order can be useful either because the requested
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 268cafb..75b9320 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -235,20 +235,30 @@ SELECT t1.c1, t2."C 1" FROM ft2 t1 JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1") OFF
 -- outer join; expressions in the clauses do not appear in equivalence class
 -- list but no output change as compared to the previous query
 EXPLAIN (VERBOSE, COSTS false)
 	SELECT t1.c1, t2."C 1" FROM ft2 t1 LEFT JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1") OFFSET 100 LIMIT 10;
 SELECT t1.c1, t2."C 1" FROM ft2 t1 LEFT JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1") OFFSET 100 LIMIT 10;
 -- A join between local table and foreign join. ORDER BY clause is added to the
 -- foreign join so that the local table can be joined using merge join strategy.
 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;
 
 -- ===================================================================
 -- WHERE with remotely-executable conditions
 -- ===================================================================
 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = 1;         -- Var, OpExpr(b), Const
 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = 100 AND t1.c2 = 0; -- BoolExpr
 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE c1 IS NULL;        -- NullTest
 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE c1 IS NOT NULL;    -- NullTest
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to