Rethink the GetForeignUpperPaths API (again).

In the previous design, the GetForeignUpperPaths FDW callback hook was
called before we got around to labeling upper relations with the proper
consider_parallel flag; this meant that any upper paths created by an FDW
would be marked not-parallel-safe.  While that's probably just as well
right now, we aren't going to want it to be true forever.  Hence, abandon
the idea that FDWs should be allowed to inject upper paths before the core
code has gotten around to creating the relevant upper relation.  (Well,
actually they still can, but it's on their own heads how well it works.)
Instead, adopt the same API already designed for create_upper_paths_hook:
we call GetForeignUpperPaths after each upperrel has been created and
populated with the paths the core planner knows how to make.

Author: tglsfdc

doc/src/sgml/fdwhandler.sgml

@@ -357,19 +357,34 @@ GetForeignJoinPaths (PlannerInfo *root,
 <programlisting>
 void
 GetForeignUpperPaths (PlannerInfo *root,
-                      RelOptInfo *scan_join_rel);
+                      UpperRelationKind stage,
+                      RelOptInfo *input_rel,
+                      RelOptInfo *output_rel);
 </programlisting>
      Create possible access paths for <firstterm>upper relation</> processing,
      which is the planner's term for all post-scan/join query processing, such
      as aggregation, window functions, sorting, and table updates.  This
      optional function is called during query planning.  Currently, it is
      called only if all base relation(s) involved in the query belong to the
      same FDW.  This function should generate <structname>ForeignPath</>
-     path(s) for the steps that the FDW knows how to perform remotely, and
-     call <function>add_path</> to add these paths to the appropriate upper
-     relation.  As with <function>GetForeignJoinPaths</>, it is not necessary
-     that this function succeed in creating any paths, since paths involving
-     local processing are always possible.
+     path(s) for any post-scan/join processing that the FDW knows how to
+     perform remotely, and call <function>add_path</> to add these paths to
+     the indicated upper relation.  As with <function>GetForeignJoinPaths</>,
+     it is not necessary that this function succeed in creating any paths,
+     since paths involving local processing are always possible.
+    </para>
+
+    <para>
+     The <literal>stage</> parameter identifies which post-scan/join step is
+     currently being considered.  <literal>output_rel</> is the upper relation
+     that should receive paths representing computation of this step,
+     and <literal>input_rel</> is the relation representing the input to this
+     step.  (Note that <structname>ForeignPath</> paths added
+     to <literal>output_rel</> would typically not have any direct dependency
+     on paths of the <literal>input_rel</>, since their processing is expected
+     to be done externally.  However, examining paths previously generated for
+     the previous processing step can be useful to avoid redundant planning
+     work.)
     </para>
 
     <para>
@@ -1530,20 +1545,20 @@ GetForeignServerByName(const char *name, bool missing_ok);
     <para>
      An FDW might additionally support direct execution of some plan actions
      that are above the level of scans and joins, such as grouping or
-     aggregation.  To offer such options, the FDW should generate paths
-     and insert them into the
-     appropriate <firstterm>upper relation</>.  For example, a path
-     representing remote aggregation should be inserted into the relation
-     obtained from <literal>fetch_upper_rel(root, UPPERREL_GROUP_AGG,
-     NULL)</>, using <function>add_path</>.  This path will be compared on a
-     cost basis with local aggregation performed by reading a simple scan path
-     for the foreign relation (note that such a path must also be supplied,
-     else there will be an error at plan time).  If the remote-aggregation
-     path wins, which it usually would, it will be converted into a plan in
-     the usual way, by calling <function>GetForeignPlan</>.
-     Usually the most convenient place to generate such paths is in
-     the <function>GetForeignUpperPaths</> callback function, although
-     it can be done earlier if that seems appropriate.
+     aggregation.  To offer such options, the FDW should generate paths and
+     insert them into the appropriate <firstterm>upper relation</>.  For
+     example, a path representing remote aggregation should be inserted into
+     the <literal>UPPERREL_GROUP_AGG</> relation, using <function>add_path</>.
+     This path will be compared on a cost basis with local aggregation
+     performed by reading a simple scan path for the foreign relation (note
+     that such a path must also be supplied, else there will be an error at
+     plan time).  If the remote-aggregation path wins, which it usually would,
+     it will be converted into a plan in the usual way, by
+     calling <function>GetForeignPlan</>.  The recommended place to generate
+     such paths is in the <function>GetForeignUpperPaths</>
+     callback function, which is called for each upper relation (i.e., each
+     post-scan/join processing step), if all the base relations of the query
+     come from the same FDW.
     </para>
 
     <para>

src/backend/optimizer/README

@@ -919,19 +919,6 @@ The result of subquery_planner() is always returned as a set of Paths
 stored in the UPPERREL_FINAL rel with NULL relids.  The other types of
 upperrels are created only if needed for the particular query.
 
-The upper-relation infrastructure is designed so that things will work
-properly if a particular upper relation is created and Paths are added
-to it sooner than would normally happen.  This allows, for example,
-for an FDW's GetForeignPaths function to insert a Path representing
-remote aggregation into the UPPERREL_GROUP_AGG upperrel, if it notices
-that the query represents an aggregation that could be done entirely on
-the foreign server.  That Path will then compete with Paths representing
-local aggregation on a regular scan of the foreign table, once the core
-planner reaches the point of considering aggregation.  (In practice,
-it will usually be more convenient for FDWs to detect such cases in a
-GetForeignUpperPaths callback; but that still represents injecting a
-Path before the core code has touched the corresponding upperrel.)
-
 
 Parallel Query and Partial Paths
 --------------------------------

src/backend/optimizer/plan/planagg.c

@@ -205,7 +205,10 @@ preprocess_minmax_aggregates(PlannerInfo *root, List *tlist)
 	 * will likely always win, but we need not assume that here.)
 	 *
 	 * Note: grouping_planner won't have created this upperrel yet, but it's
-	 * fine for us to create it first.
+	 * fine for us to create it first.  We will not have inserted the correct
+	 * consider_parallel value in it, but MinMaxAggPath paths are currently
+	 * never parallel-safe anyway, so that doesn't matter.  Likewise, it
+	 * doesn't matter that we haven't filled FDW-related fields in the rel.
 	 */
 	grouped_rel = fetch_upper_rel(root, UPPERREL_GROUP_AGG, NULL);
 	add_path(grouped_rel, (Path *)

src/backend/optimizer/plan/planner.c

@@ -1291,6 +1291,12 @@ inheritance_planner(PlannerInfo *root)
 	/* Result path must go into outer query's FINAL upperrel */
 	final_rel = fetch_upper_rel(root, UPPERREL_FINAL, NULL);
 
+	/*
+	 * We don't currently worry about setting final_rel's consider_parallel
+	 * flag in this case, nor about allowing FDWs or create_upper_paths_hook
+	 * to get control here.
+	 */
+
 	/*
 	 * If we managed to exclude every child rel, return a dummy plan; it
 	 * doesn't even need a ModifyTable node.
@@ -1788,21 +1794,6 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		root->upper_targets[UPPERREL_WINDOW] = sort_input_target;
 		root->upper_targets[UPPERREL_GROUP_AGG] = grouping_target;
 
-		/*
-		 * If there is an FDW that's responsible for the final scan/join rel,
-		 * let it consider injecting extension Paths into the query's
-		 * upperrels, where they will compete with the Paths we create below.
-		 * We pass the final scan/join rel because that's not so easily
-		 * findable from the PlannerInfo struct; anything else the FDW wants
-		 * to know should be obtainable via "root".
-		 *
-		 * Note: CustomScan providers, as well as FDWs that don't want to use
-		 * this hook, can use the create_upper_paths_hook; see below.
-		 */
-		if (current_rel->fdwroutine &&
-			current_rel->fdwroutine->GetForeignUpperPaths)
-			current_rel->fdwroutine->GetForeignUpperPaths(root, current_rel);
-
 		/*
 		 * If we have grouping and/or aggregation, consider ways to implement
 		 * that.  We build a new upperrel representing the output of this
@@ -1891,9 +1882,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 	}
 
 	/*
-	 * Now we are prepared to build the final-output upperrel.  Insert all
-	 * surviving paths, with LockRows, Limit, and/or ModifyTable steps added
-	 * if needed.
+	 * Now we are prepared to build the final-output upperrel.
 	 */
 	final_rel = fetch_upper_rel(root, UPPERREL_FINAL, NULL);
 
@@ -1910,7 +1899,15 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		final_rel->consider_parallel = true;
 
 	/*
-	 * Generate paths for the final rel.
+	 * If the current_rel belongs to a single FDW, so does the final_rel.
+	 */
+	final_rel->serverid = current_rel->serverid;
+	final_rel->umid = current_rel->umid;
+	final_rel->fdwroutine = current_rel->fdwroutine;
+
+	/*
+	 * Generate paths for the final_rel.  Insert all surviving paths, with
+	 * LockRows, Limit, and/or ModifyTable steps added if needed.
 	 */
 	foreach(lc, current_rel->pathlist)
 	{
@@ -1994,6 +1991,15 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		add_path(final_rel, path);
 	}
 
+	/*
+	 * If there is an FDW that's responsible for all baserels of the query,
+	 * let it consider adding ForeignPaths.
+	 */
+	if (final_rel->fdwroutine &&
+		final_rel->fdwroutine->GetForeignUpperPaths)
+		final_rel->fdwroutine->GetForeignUpperPaths(root, UPPERREL_FINAL,
+													current_rel, final_rel);
+
 	/* Let extensions possibly add some more paths */
 	if (create_upper_paths_hook)
 		(*create_upper_paths_hook) (root, UPPERREL_FINAL,
@@ -3268,6 +3274,13 @@ create_grouping_paths(PlannerInfo *root,
 		!has_parallel_hazard((Node *) parse->havingQual, false))
 		grouped_rel->consider_parallel = true;
 
+	/*
+	 * If the input rel belongs to a single FDW, so does the grouped rel.
+	 */
+	grouped_rel->serverid = input_rel->serverid;
+	grouped_rel->umid = input_rel->umid;
+	grouped_rel->fdwroutine = input_rel->fdwroutine;
+
 	/*
 	 * Check for degenerate grouping.
 	 */
@@ -3770,6 +3783,15 @@ create_grouping_paths(PlannerInfo *root,
 				 errmsg("could not implement GROUP BY"),
 				 errdetail("Some of the datatypes only support hashing, while others only support sorting.")));
 
+	/*
+	 * If there is an FDW that's responsible for all baserels of the query,
+	 * let it consider adding ForeignPaths.
+	 */
+	if (grouped_rel->fdwroutine &&
+		grouped_rel->fdwroutine->GetForeignUpperPaths)
+		grouped_rel->fdwroutine->GetForeignUpperPaths(root, UPPERREL_GROUP_AGG,
+													  input_rel, grouped_rel);
+
 	/* Let extensions possibly add some more paths */
 	if (create_upper_paths_hook)
 		(*create_upper_paths_hook) (root, UPPERREL_GROUP_AGG,
@@ -3820,6 +3842,13 @@ create_window_paths(PlannerInfo *root,
 		!has_parallel_hazard((Node *) activeWindows, false))
 		window_rel->consider_parallel = true;
 
+	/*
+	 * If the input rel belongs to a single FDW, so does the window rel.
+	 */
+	window_rel->serverid = input_rel->serverid;
+	window_rel->umid = input_rel->umid;
+	window_rel->fdwroutine = input_rel->fdwroutine;
+
 	/*
 	 * Consider computing window functions starting from the existing
 	 * cheapest-total path (which will likely require a sort) as well as any
@@ -3841,6 +3870,15 @@ create_window_paths(PlannerInfo *root,
 								   activeWindows);
 	}
 
+	/*
+	 * If there is an FDW that's responsible for all baserels of the query,
+	 * let it consider adding ForeignPaths.
+	 */
+	if (window_rel->fdwroutine &&
+		window_rel->fdwroutine->GetForeignUpperPaths)
+		window_rel->fdwroutine->GetForeignUpperPaths(root, UPPERREL_WINDOW,
+													 input_rel, window_rel);
+
 	/* Let extensions possibly add some more paths */
 	if (create_upper_paths_hook)
 		(*create_upper_paths_hook) (root, UPPERREL_WINDOW,
@@ -3984,6 +4022,13 @@ create_distinct_paths(PlannerInfo *root,
 	 */
 	distinct_rel->consider_parallel = input_rel->consider_parallel;
 
+	/*
+	 * If the input rel belongs to a single FDW, so does the distinct_rel.
+	 */
+	distinct_rel->serverid = input_rel->serverid;
+	distinct_rel->umid = input_rel->umid;
+	distinct_rel->fdwroutine = input_rel->fdwroutine;
+
 	/* Estimate number of distinct rows there will be */
 	if (parse->groupClause || parse->groupingSets || parse->hasAggs ||
 		root->hasHavingQual)
@@ -4129,6 +4174,15 @@ create_distinct_paths(PlannerInfo *root,
 				 errmsg("could not implement DISTINCT"),
 				 errdetail("Some of the datatypes only support hashing, while others only support sorting.")));
 
+	/*
+	 * If there is an FDW that's responsible for all baserels of the query,
+	 * let it consider adding ForeignPaths.
+	 */
+	if (distinct_rel->fdwroutine &&
+		distinct_rel->fdwroutine->GetForeignUpperPaths)
+		distinct_rel->fdwroutine->GetForeignUpperPaths(root, UPPERREL_DISTINCT,
+													input_rel, distinct_rel);
+
 	/* Let extensions possibly add some more paths */
 	if (create_upper_paths_hook)
 		(*create_upper_paths_hook) (root, UPPERREL_DISTINCT,
@@ -4176,6 +4230,13 @@ create_ordered_paths(PlannerInfo *root,
 		!has_parallel_hazard((Node *) target->exprs, false))
 		ordered_rel->consider_parallel = true;
 
+	/*
+	 * If the input rel belongs to a single FDW, so does the ordered_rel.
+	 */
+	ordered_rel->serverid = input_rel->serverid;
+	ordered_rel->umid = input_rel->umid;
+	ordered_rel->fdwroutine = input_rel->fdwroutine;
+
 	foreach(lc, input_rel->pathlist)
 	{
 		Path	   *path = (Path *) lfirst(lc);
@@ -4204,6 +4265,15 @@ create_ordered_paths(PlannerInfo *root,
 		}
 	}
 
+	/*
+	 * If there is an FDW that's responsible for all baserels of the query,
+	 * let it consider adding ForeignPaths.
+	 */
+	if (ordered_rel->fdwroutine &&
+		ordered_rel->fdwroutine->GetForeignUpperPaths)
+		ordered_rel->fdwroutine->GetForeignUpperPaths(root, UPPERREL_ORDERED,
+													  input_rel, ordered_rel);
+
 	/* Let extensions possibly add some more paths */
 	if (create_upper_paths_hook)
 		(*create_upper_paths_hook) (root, UPPERREL_ORDERED,

src/backend/optimizer/prep/prepunion.c

@@ -174,6 +174,10 @@ plan_set_operations(PlannerInfo *root)
 	 */
 	setop_rel = fetch_upper_rel(root, UPPERREL_SETOP, NULL);
 
+	/*
+	 * We don't currently worry about setting setop_rel's consider_parallel
+	 * flag, nor about allowing FDWs to contribute paths to it.
+	 */
 
 	/*
 	 * If the topmost node is a recursive union, it needs special processing.

src/include/foreign/fdwapi.h

@@ -60,7 +60,9 @@ typedef void (*GetForeignJoinPaths_function) (PlannerInfo *root,
 												   JoinPathExtraData *extra);
 
 typedef void (*GetForeignUpperPaths_function) (PlannerInfo *root,
-												  RelOptInfo *scan_join_rel);
+													 UpperRelationKind stage,
+													   RelOptInfo *input_rel,
+													 RelOptInfo *output_rel);
 
 typedef void (*AddForeignUpdateTargets_function) (Query *parsetree,
 												   RangeTblEntry *target_rte,