Thanks Rushabh for your review and comments.

On Thu, Nov 26, 2015 at 5:39 PM, Rushabh Lathia <rushabh.lat...@gmail.com>
wrote:

> Hi Ashutosh,
>
> I reviewed your latest version of patch and over all the implementation
> and other details look good to me.
>
> Here are few cosmetic issues which I found:
>
> 1) Patch not getting applied cleanly - white space warning
>
>
Done.


> 2)
>
> -    List       *usable_pathkeys = NIL;
> +    List        *useful_pathkeys_list = NIL;    /* List of all pathkeys */
>
> Code alignment is not correct with other declared variables.
>
>
Incorporated the change in the patch.

3)
>
> +        {
> +            PathKey    *pathkey;
> +            List    *pathkeys;
> +
> +            pathkey = make_canonical_pathkey(root, cur_ec,
> +
> linitial_oid(cur_ec->ec_opfamilies),
> +                                            BTLessStrategyNumber,
> +                                            false);
> +            pathkeys = list_make1(pathkey);
> +            useful_pathkeys_list = lappend(useful_pathkeys_list,
> pathkeys);
> +        }
>
> Code alignment need to fix at make_canonical_pathkey().
>

Incorporated the change in the patch.

I have also removed the TODO item in the prologue of this function, since
none has objected to externalization of make_canonical_pathkeys till now
and it's not expected to be part of the final commit.


>
> 4)
>
> I don't understand the meaning of following added testcase into
> postgres_fdw.
>
> +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
> @@ -171,20 +171,35 @@ SELECT COUNT(*) FROM ft1 t1;
>  -- join two tables
>  SELECT t1.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3,
> t1.c1 OFFSET 100 LIMIT 10;
>  -- subquery
>  SELECT * FROM ft1 t1 WHERE t1.c3 IN (SELECT c3 FROM ft2 t2 WHERE c1 <=
> 10) ORDER BY c1;
>  -- subquery+MAX
>  SELECT * FROM ft1 t1 WHERE t1.c3 = (SELECT MAX(c3) FROM ft2 t2) ORDER BY
> c1;
>  -- used in CTE
>  WITH t1 AS (SELECT * FROM ft1 WHERE c1 <= 10) SELECT t2.c1, t2.c2, t2.c3,
> t2.c4 FROM t1, ft2 t2 WHERE t1.c1 = t2.c1 ORDER BY t1.c1;
>  -- fixed values
>  SELECT 'fixed', NULL FROM ft1 t1 WHERE c1 = 1;
> +-- getting data sorted from the foreign table for merge join
> +-- Since we are interested in merge join, disable other joins
> +SET enable_hashjoin TO false;
> +SET enable_nestloop TO false;
> +-- inner join, expressions in the clauses appear in the equivalence class
> list
> +EXPLAIN (VERBOSE, COSTS false)
> +    SELECT t1.c1, t2."C 1" FROM ft2 t1 JOIN "S 1"."T 1" t2 ON (t1.c1 =
> t2."C 1") ORDER BY t1.c1 OFFSET 100 LIMIT 10;
> +SELECT t1.c1, t2."C 1" FROM ft2 t1 JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C
> 1") ORDER BY t1.c1 OFFSET 100 LIMIT 10;
> +-- outer join, expression 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") ORDER BY t1.c1 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") ORDER BY t1.c1 OFFSET 100 LIMIT 10;
> +SET enable_hashjoin TO true;
> +SET enable_nestloop TO true;
>
> Because, I removed the code changes of the patch and then I run the test
> seem like it has nothing to do with the code changes. Above set of test
> giving
> same result with/without patch.
>
> Am I missing something ?
>

Actually, the output of merge join is always ordered by the pathkeys used
for merge join. That routed through LIMIT node remains ordered. So, we
actually do not need ORDER BY t1.c1 clause in the above queries. Without
that clause, the tests will show difference output with and without patch.
I have changed the attached patch accordingly.


>
> Apart from this I debugged the patch for each scenario (query pathkeys and
> pathkeys arising out of the equivalence classes) and so far patch looks
> good
> to me.
>
>
Thanks.


> Attaching update version of patch by fixing the cosmetic changes.
>
>
Attached version of patch contains your changes.
-- 
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 866a09b..a9b0bd8 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -336,20 +336,93 @@ WITH t1 AS (SELECT * FROM ft1 WHERE c1 <= 10) SELECT t2.c1, t2.c2, t2.c3, t2.c4
  10 |  0 | 00010 | Sun Jan 11 00:00:00 1970 PST
 (10 rows)
 
 -- fixed values
 SELECT 'fixed', NULL FROM ft1 t1 WHERE c1 = 1;
  ?column? | ?column? 
 ----------+----------
  fixed    | 
 (1 row)
 
+-- getting data sorted from the foreign table for merge join
+-- Since we are interested in merge join, disable other joins
+-- Merge join always produces the result in the sorted order, so no need for
+-- separate ORDER BY clause to get ordered results.
+SET enable_hashjoin TO false;
+SET enable_nestloop TO false;
+-- inner join, expressions in the clauses appear in the equivalence class list
+EXPLAIN (VERBOSE, COSTS false)
+	SELECT t1.c1, t2."C 1" FROM ft2 t1 JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1") OFFSET 100 LIMIT 10;
+                                 QUERY PLAN                                 
+----------------------------------------------------------------------------
+ Limit
+   Output: t1.c1, t2."C 1"
+   ->  Merge Join
+         Output: t1.c1, t2."C 1"
+         Merge Cond: (t1.c1 = t2."C 1")
+         ->  Foreign Scan on public.ft2 t1
+               Output: t1.c1
+               Remote SQL: SELECT "C 1" FROM "S 1"."T 1" ORDER BY "C 1" ASC
+         ->  Index Only Scan using t1_pkey on "S 1"."T 1" t2
+               Output: t2."C 1"
+(10 rows)
+
+SELECT t1.c1, t2."C 1" FROM ft2 t1 JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1") OFFSET 100 LIMIT 10;
+ c1  | C 1 
+-----+-----
+ 101 | 101
+ 102 | 102
+ 103 | 103
+ 104 | 104
+ 105 | 105
+ 106 | 106
+ 107 | 107
+ 108 | 108
+ 109 | 109
+ 110 | 110
+(10 rows)
+
+-- outer join, expression 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;
+                                 QUERY PLAN                                 
+----------------------------------------------------------------------------
+ Limit
+   Output: t1.c1, t2."C 1"
+   ->  Merge Left Join
+         Output: t1.c1, t2."C 1"
+         Merge Cond: (t1.c1 = t2."C 1")
+         ->  Foreign Scan on public.ft2 t1
+               Output: t1.c1
+               Remote SQL: SELECT "C 1" FROM "S 1"."T 1" ORDER BY "C 1" ASC
+         ->  Index Only Scan using t1_pkey on "S 1"."T 1" t2
+               Output: t2."C 1"
+(10 rows)
+
+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;
+ c1  | C 1 
+-----+-----
+ 101 | 101
+ 102 | 102
+ 103 | 103
+ 104 | 104
+ 105 | 105
+ 106 | 106
+ 107 | 107
+ 108 | 108
+ 109 | 109
+ 110 | 110
+(10 rows)
+
+SET enable_hashjoin TO true;
+SET enable_nestloop TO true;
 -- ===================================================================
 -- 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
    Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" = 1))
 (3 rows)
@@ -3531,20 +3604,119 @@ select tableoid::regclass, * from bar order by 1,2;
  tableoid | f1 | f2  
 ----------+----+-----
  bar      |  1 | 211
  bar      |  2 | 222
  bar      |  6 | 166
  bar2     |  3 | 233
  bar2     |  4 | 244
  bar2     |  7 | 177
 (6 rows)
 
+-- Tests for sort pushdown for merge joins for foreign children.
+-- Fill enough data in the tables, so that getting data sorted from the foreign
+-- server is cheaper than sorting it locally.
+truncate table loct1;
+truncate table only foo;
+\set num_rows_foo 2000
+insert into loct1 select generate_series(0, :num_rows_foo, 2), generate_series(0, :num_rows_foo, 2), generate_series(0, :num_rows_foo, 2);
+insert into foo select generate_series(1, :num_rows_foo, 2), generate_series(1, :num_rows_foo, 2);
+set enable_hashjoin to false;
+set enable_nestloop to false;
+-- Sort pushdown for merge join is enabled only when use_remote_estimate is
+-- true.
+alter foreign table foo2 options (use_remote_estimate 'true');
+-- Create indexes so that sorted joining relations cost less.
+create index i_loct1_f1 on loct1(f1);
+create index i_foo_f1 on foo(f1);
+analyze foo;
+analyze loct1;
+-- inner join, expressions in the clauses appear in the equivalence class list
+explain (verbose, costs off)
+	select foo.f1, loct1.f1 from foo join loct1 on (foo.f1 = loct1.f1) order by foo.f2 offset 10 limit 10;
+                                      QUERY PLAN                                       
+---------------------------------------------------------------------------------------
+ Limit
+   Output: foo.f1, loct1.f1, foo.f2
+   ->  Sort
+         Output: foo.f1, loct1.f1, foo.f2
+         Sort Key: foo.f2
+         ->  Merge Join
+               Output: foo.f1, loct1.f1, foo.f2
+               Merge Cond: (foo.f1 = loct1.f1)
+               ->  Merge Append
+                     Sort Key: foo.f1
+                     ->  Index Scan using i_foo_f1 on public.foo
+                           Output: foo.f1, foo.f2
+                     ->  Foreign Scan on public.foo2
+                           Output: foo2.f1, foo2.f2
+                           Remote SQL: SELECT f1, f2 FROM public.loct1 ORDER BY f1 ASC
+               ->  Index Only Scan using i_loct1_f1 on public.loct1
+                     Output: loct1.f1
+(17 rows)
+
+select foo.f1, loct1.f1 from foo join loct1 on (foo.f1 = loct1.f1) order by foo.f2 offset 10 limit 10;
+ f1 | f1 
+----+----
+ 20 | 20
+ 22 | 22
+ 24 | 24
+ 26 | 26
+ 28 | 28
+ 30 | 30
+ 32 | 32
+ 34 | 34
+ 36 | 36
+ 38 | 38
+(10 rows)
+
+-- outer join, expression in the clauses do not appear in equivalence class list
+-- but no output change as compared to the previous query
+explain (verbose, costs off)
+	select foo.f1, loct1.f1 from foo left join loct1 on (foo.f1 = loct1.f1) order by foo.f2 offset 10 limit 10;
+                                      QUERY PLAN                                       
+---------------------------------------------------------------------------------------
+ Limit
+   Output: foo.f1, loct1.f1, foo.f2
+   ->  Sort
+         Output: foo.f1, loct1.f1, foo.f2
+         Sort Key: foo.f2
+         ->  Merge Left Join
+               Output: foo.f1, loct1.f1, foo.f2
+               Merge Cond: (foo.f1 = loct1.f1)
+               ->  Merge Append
+                     Sort Key: foo.f1
+                     ->  Index Scan using i_foo_f1 on public.foo
+                           Output: foo.f1, foo.f2
+                     ->  Foreign Scan on public.foo2
+                           Output: foo2.f1, foo2.f2
+                           Remote SQL: SELECT f1, f2 FROM public.loct1 ORDER BY f1 ASC
+               ->  Index Only Scan using i_loct1_f1 on public.loct1
+                     Output: loct1.f1
+(17 rows)
+
+select foo.f1, loct1.f1 from foo left join loct1 on (foo.f1 = loct1.f1) order by foo.f2 offset 10 limit 10;
+ f1 | f1 
+----+----
+ 10 | 10
+ 11 |   
+ 12 | 12
+ 13 |   
+ 14 | 14
+ 15 |   
+ 16 | 16
+ 17 |   
+ 18 | 18
+ 19 |   
+(10 rows)
+
+set enable_hashjoin to true;
+set enable_nestloop to true;
 -- Test that WHERE CURRENT OF is not supported
 begin;
 declare c cursor for select * from bar where f1 = 7;
 fetch from c;
  f1 | f2  
 ----+-----
   7 | 177
 (1 row)
 
 update bar set f2 = null where current of c;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index cd4ed0c..88dcb22 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -251,20 +251,21 @@ static void postgresExplainForeignScan(ForeignScanState *node,
 static void postgresExplainForeignModify(ModifyTableState *mtstate,
 							 ResultRelInfo *rinfo,
 							 List *fdw_private,
 							 int subplan_index,
 							 ExplainState *es);
 static bool postgresAnalyzeForeignTable(Relation relation,
 							AcquireSampleRowsFunc *func,
 							BlockNumber *totalpages);
 static List *postgresImportForeignSchema(ImportForeignSchemaStmt *stmt,
 							Oid serverOid);
+static List *get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel);
 
 /*
  * Helper functions
  */
 static void estimate_path_cost_size(PlannerInfo *root,
 						RelOptInfo *baserel,
 						List *join_conds,
 						List *pathkeys,
 						double *p_rows, int *p_width,
 						Cost *p_startup_cost, Cost *p_total_cost);
@@ -501,100 +502,234 @@ postgresGetForeignRelSize(PlannerInfo *root,
 		set_baserel_size_estimates(root, baserel);
 
 		/* Fill in basically-bogus cost estimates for use later. */
 		estimate_path_cost_size(root, baserel, NIL, NIL,
 								&fpinfo->rows, &fpinfo->width,
 								&fpinfo->startup_cost, &fpinfo->total_cost);
 	}
 }
 
 /*
+ * get_useful_pathkeys_for_relation
+ * Function collects useful pathkeys for given relation. The caller is
+ * possibly going to create paths with these pathkeys so as to get the data
+ * sorted according to the pathkeys.
+ * Following pathkeys are of interest here.
+ * 1. query pathkeys
+ * 2. pathkeys arising out of the equivalence classes
+ * Comments in the function explain these two in details.
+ */
+static List *
+get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel)
+{
+	List	*useful_pathkeys_list = NIL; /* List of pathkeys to be returned */
+	bool	useful_query_pathkeys;
+	List	*useful_eclass_list = NIL;	/* List of unique eclasses */
+	PathKey	*first_query_pathkey = NULL;
+	PgFdwRelationInfo	*fpinfo = (PgFdwRelationInfo *) rel->fdw_private;
+	ListCell	*lc;
+
+	/*
+	 * Determine whether we can potentially push query pathkeys to the remote
+	 * side, avoiding a local sort.
+	 */
+	if (root->query_pathkeys)
+	{
+		/* By default assume that root->query_pathkeys is pushable. */
+		useful_query_pathkeys = true;
+		foreach(lc, root->query_pathkeys)
+		{
+			PathKey    *pathkey = (PathKey *) lfirst(lc);
+			EquivalenceClass *pathkey_ec = pathkey->pk_eclass;
+			Expr	   *em_expr;
+
+			/*
+			 * is_foreign_expr would detect volatile expressions as well, but
+			 * ec_has_volatile saves some cycles.
+			 */
+			if (pathkey_ec->ec_has_volatile ||
+				!(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
+				!is_foreign_expr(root, rel, em_expr))
+				{
+					/*
+					 * The planner and executor don't have any clever strategy
+					 * for taking data sorted by a prefix of the query's
+					 * pathkeys and getting it to be sorted by all of those
+					 * pathekeys. We'll just end up resorting the entire data
+					 * set.  So, unless we can push down all of the query
+					 * pathkeys, forget it.
+					 */
+					useful_query_pathkeys = false;
+				}
+		}
+
+		if (useful_query_pathkeys)
+			useful_pathkeys_list = list_make1(list_copy(root->query_pathkeys));
+	}
+
+	/*
+	 * Next we create single member pathkeys useful for merge joins. Choosing
+	 * wrong join plan based on speculative costs can cause serious problems.
+	 * Create corresponding paths only when we can estimate costs with more
+	 * precision i.e. get those from foreign server.
+	 */
+	if (!fpinfo->use_remote_estimate)
+		return useful_pathkeys_list;
+
+	/*
+	 * Check equivalence classes where this relation appears. Getting data
+	 * ordered on corresponding expressions might help merge join. A join
+	 * between two relations might have multiple merge joinable clauses, thus
+	 * fetching foreign data sorted on all the expressions involved optimizes
+	 * merge join locally. But we do not have knowledge about the indexes
+	 * present on the foreign server. The cost of sorting data on multiple
+	 * expressions can vary greatly based on the kind of expressions and their
+	 * presence in an index. Hence for now, we create paths with single pathkey.
+	 * Nothing to do if the relation is not part of any equivalence class.
+	 */
+	if (rel->has_eclass_joins)
+	{
+		foreach(lc, root->eq_classes)
+		{
+			EquivalenceClass *cur_ec = (EquivalenceClass *) lfirst(lc);
+
+			if (eclass_useful_for_merging(root, cur_ec, rel))
+				useful_eclass_list = lappend(useful_eclass_list, cur_ec);
+		}
+	}
+
+	foreach (lc, rel->joininfo)
+	{
+		Relids relids;
+		RestrictInfo *restrictinfo = (RestrictInfo *) lfirst(lc);
+
+		/*
+		 * Equivalence classes are marked by the parent relations and not child.
+		 * If specified rel is a child, we must consider the topmost parent rel.
+		 */
+		if (rel->reloptkind == RELOPT_OTHER_MEMBER_REL)
+			relids = find_childrel_top_parent(root, rel)->relids;
+		else
+			relids = rel->relids;
+
+
+		/* Consider only mergejoinable clauses */
+		if (restrictinfo->mergeopfamilies == NIL)
+			continue;
+
+		update_mergeclause_eclasses(root, restrictinfo);
+
+		/* Pick up the equivalence class corresponding to the given relation */
+		if (bms_is_subset(restrictinfo->right_ec->ec_relids, relids))
+			useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
+														restrictinfo->right_ec);
+		else
+		{
+			/*
+			 * One of left or right should belong correspond to the relation we
+			 * are looking for.
+			 */
+			Assert(bms_is_subset(restrictinfo->left_ec->ec_relids, relids));
+			useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
+														restrictinfo->left_ec);
+		}
+	}
+
+	/*
+	 * Scan through the distinct equivalence classes collected and create
+	 * one pathkey for each of them if the equivalence member belonging to the
+	 * given relation is pushable to the foreign server.
+	 * Also check if the equivalence class is different from the equivalence
+	 * class of single member root->query_pathkeys, in which case, we already
+	 * have already considered that pathkey.
+	 */
+	if (list_length(root->query_pathkeys) == 1)
+		first_query_pathkey = linitial(root->query_pathkeys);
+
+	foreach(lc, useful_eclass_list)
+	{
+		EquivalenceClass *cur_ec = lfirst(lc);
+		Expr	*em_expr;
+
+		if (first_query_pathkey &&
+			cur_ec == first_query_pathkey->pk_eclass)
+			continue;
+
+		if ((em_expr = find_em_expr_for_rel(cur_ec, rel)) &&
+				is_foreign_expr(root, rel, em_expr))
+		{
+			PathKey	*pathkey;
+			List	*pathkeys;
+
+			pathkey = make_canonical_pathkey(root, cur_ec,
+											 linitial_oid(cur_ec->ec_opfamilies),
+											 BTLessStrategyNumber,
+											 false);
+			pathkeys = list_make1(pathkey);
+			useful_pathkeys_list = lappend(useful_pathkeys_list, pathkeys);
+		}
+	}
+
+	return useful_pathkeys_list;
+}
+
+/*
  * postgresGetForeignPaths
  *		Create possible scan paths for a scan on the foreign table
  */
 static void
 postgresGetForeignPaths(PlannerInfo *root,
 						RelOptInfo *baserel,
 						Oid foreigntableid)
 {
 	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) baserel->fdw_private;
 	ForeignPath *path;
 	List	   *ppi_list;
 	ListCell   *lc;
-	List	   *usable_pathkeys = NIL;
+	List	   *useful_pathkeys_list = NIL;	/* List of all pathkeys */
 
 	/*
 	 * Create simplest ForeignScan path node and add it to baserel.  This path
 	 * corresponds to SeqScan path of regular tables (though depending on what
 	 * baserestrict conditions we were able to send to remote, there might
 	 * actually be an indexscan happening there).  We already did all the work
 	 * to estimate cost and size of this path.
 	 */
 	path = create_foreignscan_path(root, baserel,
 								   fpinfo->rows,
 								   fpinfo->startup_cost,
 								   fpinfo->total_cost,
 								   NIL, /* no pathkeys */
 								   NULL,		/* no outer rel either */
 								   NIL);		/* no fdw_private list */
 	add_path(baserel, (Path *) path);
 
-	/*
-	 * Determine whether we can potentially push query pathkeys to the remote
-	 * side, avoiding a local sort.
-	 */
-	foreach(lc, root->query_pathkeys)
-	{
-		PathKey    *pathkey = (PathKey *) lfirst(lc);
-		EquivalenceClass *pathkey_ec = pathkey->pk_eclass;
-		Expr	   *em_expr;
-
-		/*
-		 * is_foreign_expr would detect volatile expressions as well, but
-		 * ec_has_volatile saves some cycles.
-		 */
-		if (!pathkey_ec->ec_has_volatile &&
-			(em_expr = find_em_expr_for_rel(pathkey_ec, baserel)) &&
-			is_foreign_expr(root, baserel, em_expr))
-			usable_pathkeys = lappend(usable_pathkeys, pathkey);
-		else
-		{
-			/*
-			 * The planner and executor don't have any clever strategy for
-			 * taking data sorted by a prefix of the query's pathkeys and
-			 * getting it to be sorted by all of those pathekeys.  We'll just
-			 * end up resorting the entire data set.  So, unless we can push
-			 * down all of the query pathkeys, forget it.
-			 */
-			list_free(usable_pathkeys);
-			usable_pathkeys = NIL;
-			break;
-		}
-	}
+	useful_pathkeys_list = get_useful_pathkeys_for_relation(root, baserel);
 
-	/* Create a path with useful pathkeys, if we found one. */
-	if (usable_pathkeys != NULL)
+	/* Create one path for each set of pathkeys we found above. */
+	foreach (lc, useful_pathkeys_list)
 	{
 		double		rows;
 		int			width;
 		Cost		startup_cost;
 		Cost		total_cost;
+		List		*useful_pathkeys = lfirst(lc);
 
-		estimate_path_cost_size(root, baserel, NIL, usable_pathkeys,
+		estimate_path_cost_size(root, baserel, NIL, useful_pathkeys,
 								&rows, &width, &startup_cost, &total_cost);
 
 		add_path(baserel, (Path *)
 				 create_foreignscan_path(root, baserel,
 										 rows,
 										 startup_cost,
 										 total_cost,
-										 usable_pathkeys,
+										 useful_pathkeys,
 										 NULL,
 										 NIL));
 	}
 
 	/*
 	 * If we're not using remote estimates, stop here.  We have no way to
 	 * estimate whether any join clauses would be worth sending across, so
 	 * don't bother building parameterized paths.
 	 */
 	if (!fpinfo->use_remote_estimate)
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index f243de8..d13a244 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -106,17 +106,17 @@ extern void deparseUpdateSql(StringInfo buf, PlannerInfo *root,
 extern void deparseDeleteSql(StringInfo buf, PlannerInfo *root,
 				 Index rtindex, Relation rel,
 				 List *returningList,
 				 List **retrieved_attrs);
 extern void deparseAnalyzeSizeSql(StringInfo buf, Relation rel);
 extern void deparseAnalyzeSql(StringInfo buf, Relation rel,
 				  List **retrieved_attrs);
 extern void deparseStringLiteral(StringInfo buf, const char *val);
 extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
 extern void appendOrderByClause(StringInfo buf, PlannerInfo *root,
-					RelOptInfo *baserel, List *pathkeys);
+								RelOptInfo *baserel, List *pathkeys);
 
 /* in shippable.c */
 extern bool is_builtin(Oid objectId);
 extern bool is_shippable(Oid objectId, Oid classId, PgFdwRelationInfo *fpinfo);
 
 #endif   /* POSTGRES_FDW_H */
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 671e38c..49e48a4 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -171,20 +171,37 @@ SELECT COUNT(*) FROM ft1 t1;
 -- join two tables
 SELECT t1.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10;
 -- subquery
 SELECT * FROM ft1 t1 WHERE t1.c3 IN (SELECT c3 FROM ft2 t2 WHERE c1 <= 10) ORDER BY c1;
 -- subquery+MAX
 SELECT * FROM ft1 t1 WHERE t1.c3 = (SELECT MAX(c3) FROM ft2 t2) ORDER BY c1;
 -- used in CTE
 WITH t1 AS (SELECT * FROM ft1 WHERE c1 <= 10) SELECT t2.c1, t2.c2, t2.c3, t2.c4 FROM t1, ft2 t2 WHERE t1.c1 = t2.c1 ORDER BY t1.c1;
 -- fixed values
 SELECT 'fixed', NULL FROM ft1 t1 WHERE c1 = 1;
+-- getting data sorted from the foreign table for merge join
+-- Since we are interested in merge join, disable other joins
+-- Merge join always produces the result in the sorted order, so no need for
+-- separate ORDER BY clause to get ordered results.
+SET enable_hashjoin TO false;
+SET enable_nestloop TO false;
+-- inner join, expressions in the clauses appear in the equivalence class list
+EXPLAIN (VERBOSE, COSTS false)
+	SELECT t1.c1, t2."C 1" FROM ft2 t1 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 JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1") OFFSET 100 LIMIT 10;
+-- outer join, expression 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;
+SET enable_hashjoin TO true;
+SET enable_nestloop TO true;
 
 -- ===================================================================
 -- 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
 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE round(abs(c1), 0) = 1; -- FuncExpr
 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE c1 = -c1;          -- OpExpr(l)
@@ -805,20 +822,50 @@ update bar set f2 = f2 + 100
 from
   ( select f1 from foo union all select f1+3 from foo ) ss
 where bar.f1 = ss.f1;
 update bar set f2 = f2 + 100
 from
   ( select f1 from foo union all select f1+3 from foo ) ss
 where bar.f1 = ss.f1;
 
 select tableoid::regclass, * from bar order by 1,2;
 
+-- Tests for sort pushdown for merge joins for foreign children.
+-- Fill enough data in the tables, so that getting data sorted from the foreign
+-- server is cheaper than sorting it locally.
+truncate table loct1;
+truncate table only foo;
+\set num_rows_foo 2000
+insert into loct1 select generate_series(0, :num_rows_foo, 2), generate_series(0, :num_rows_foo, 2), generate_series(0, :num_rows_foo, 2);
+insert into foo select generate_series(1, :num_rows_foo, 2), generate_series(1, :num_rows_foo, 2);
+set enable_hashjoin to false;
+set enable_nestloop to false;
+-- Sort pushdown for merge join is enabled only when use_remote_estimate is
+-- true.
+alter foreign table foo2 options (use_remote_estimate 'true');
+-- Create indexes so that sorted joining relations cost less.
+create index i_loct1_f1 on loct1(f1);
+create index i_foo_f1 on foo(f1);
+analyze foo;
+analyze loct1;
+-- inner join, expressions in the clauses appear in the equivalence class list
+explain (verbose, costs off)
+	select foo.f1, loct1.f1 from foo join loct1 on (foo.f1 = loct1.f1) order by foo.f2 offset 10 limit 10;
+select foo.f1, loct1.f1 from foo join loct1 on (foo.f1 = loct1.f1) order by foo.f2 offset 10 limit 10;
+-- outer join, expression in the clauses do not appear in equivalence class list
+-- but no output change as compared to the previous query
+explain (verbose, costs off)
+	select foo.f1, loct1.f1 from foo left join loct1 on (foo.f1 = loct1.f1) order by foo.f2 offset 10 limit 10;
+select foo.f1, loct1.f1 from foo left join loct1 on (foo.f1 = loct1.f1) order by foo.f2 offset 10 limit 10;
+set enable_hashjoin to true;
+set enable_nestloop to true;
+
 -- Test that WHERE CURRENT OF is not supported
 begin;
 declare c cursor for select * from bar where f1 = 7;
 fetch from c;
 update bar set f2 = null where current of c;
 rollback;
 
 drop table foo cascade;
 drop table bar cascade;
 drop table loct1;
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index c6b5d78..b81cc49 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -21,43 +21,40 @@
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "nodes/plannodes.h"
 #include "optimizer/clauses.h"
 #include "optimizer/pathnode.h"
 #include "optimizer/paths.h"
 #include "optimizer/tlist.h"
 #include "utils/lsyscache.h"
 
 
-static PathKey *make_canonical_pathkey(PlannerInfo *root,
-					   EquivalenceClass *eclass, Oid opfamily,
-					   int strategy, bool nulls_first);
 static bool pathkey_is_redundant(PathKey *new_pathkey, List *pathkeys);
 static bool right_merge_direction(PlannerInfo *root, PathKey *pathkey);
 
 
 /****************************************************************************
  *		PATHKEY CONSTRUCTION AND REDUNDANCY TESTING
  ****************************************************************************/
 
 /*
  * make_canonical_pathkey
  *	  Given the parameters for a PathKey, find any pre-existing matching
  *	  pathkey in the query's list of "canonical" pathkeys.  Make a new
  *	  entry if there's not one already.
  *
  * Note that this function must not be used until after we have completed
  * merging EquivalenceClasses.  (We don't try to enforce that here; instead,
  * equivclass.c will complain if a merge occurs after root->canon_pathkeys
  * has become nonempty.)
  */
-static PathKey *
+PathKey *
 make_canonical_pathkey(PlannerInfo *root,
 					   EquivalenceClass *eclass, Oid opfamily,
 					   int strategy, bool nulls_first)
 {
 	PathKey    *pk;
 	ListCell   *lc;
 	MemoryContext oldcontext;
 
 	/* The passed eclass might be non-canonical, so chase up to the top */
 	while (eclass->ec_merged)
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 87123a5..5ef6e1a 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -197,12 +197,15 @@ extern List *find_mergeclauses_for_pathkeys(PlannerInfo *root,
 extern List *select_outer_pathkeys_for_merge(PlannerInfo *root,
 								List *mergeclauses,
 								RelOptInfo *joinrel);
 extern List *make_inner_pathkeys_for_merge(PlannerInfo *root,
 							  List *mergeclauses,
 							  List *outer_pathkeys);
 extern List *truncate_useless_pathkeys(PlannerInfo *root,
 						  RelOptInfo *rel,
 						  List *pathkeys);
 extern bool has_useful_pathkeys(PlannerInfo *root, RelOptInfo *rel);
+extern PathKey *make_canonical_pathkey(PlannerInfo *root,
+					   EquivalenceClass *eclass, Oid opfamily,
+					   int strategy, bool nulls_first);
 
 #endif   /* PATHS_H */
-- 
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