Initial code review for CustomScan patch.

Get rid of the pernicious entanglement between planner and executor headers
introduced by commit 0b03e5951bf0a1a8868db13f02049cf686a82165.

Also, rearrange the CustomFoo struct/typedef definitions so that all the
typedef names are seen as used by the compiler.  Without this pgindent
will mess things up a bit, which is not so important perhaps, but it also
removes a bizarre discrepancy between the declaration arrangement used for
CustomExecMethods and that used for CustomScanMethods and
CustomPathMethods.

Clean up the commentary around ExecSupportsMarkRestore to reflect the
rather large change in its API.

Const-ify register_custom_path_provider's argument.  This necessitates
casting away const in the function, but that seems better than forcing
callers of the function to do so (or else not const-ify their method
pointer structs, which was sort of the whole point).

De-export fix_expr_common.  I don't like the exporting of fix_scan_expr
or replace_nestloop_params either, but this one surely has got little
excuse.

3 files changed, 64 insertions, 61 deletions

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 0a85cd99061..e6bd4c326ca 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -78,8 +78,8 @@ static WorkTableScan *create_worktablescan_plan(PlannerInfo *root, Path *best_pa
 static ForeignScan *create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
 						List *tlist, List *scan_clauses);
 static Plan *create_customscan_plan(PlannerInfo *root,
-									CustomPath *best_path,
-									List *tlist, List *scan_clauses);
+					   CustomPath *best_path,
+					   List *tlist, List *scan_clauses);
 static NestLoop *create_nestloop_plan(PlannerInfo *root, NestPath *best_path,
 					 Plan *outer_plan, Plan *inner_plan);
 static MergeJoin *create_mergejoin_plan(PlannerInfo *root, MergePath *best_path,
@@ -1083,52 +1083,6 @@ create_unique_plan(PlannerInfo *root, UniquePath *best_path)
 	return plan;
 }
 
-/*
- * create_custom_plan
- *
- * Transform a CustomPath into a Plan.
- */
-static Plan *
-create_customscan_plan(PlannerInfo *root, CustomPath *best_path,
-					   List *tlist, List *scan_clauses)
-{
-	Plan		   *plan;
-	RelOptInfo	   *rel = best_path->path.parent;
-
-	/*
-	 * Right now, all we can support is CustomScan node which is associated
-	 * with a particular base relation to be scanned.
-	 */
-	Assert(rel && rel->reloptkind == RELOPT_BASEREL);
-
-	/*
-	 * Sort clauses into the best execution order, although custom-scan
-	 * provider can reorder them again.
-	 */
-	scan_clauses = order_qual_clauses(root, scan_clauses);
-
-	/*
-	 * Create a CustomScan (or its inheritance) node according to
-	 * the supplied CustomPath.
-	 */
-	plan = best_path->methods->PlanCustomPath(root, rel, best_path, tlist,
-											  scan_clauses);
-
-	/*
-	 * NOTE: unlike create_foreignscan_plan(), it is responsibility of
-	 * the custom plan provider to replace outer-relation variables
-	 * with nestloop params, because we cannot know how many expression
-	 * trees are held in the private fields.
-	 */
-
-	/*
-	 * Copy cost data from Path to Plan; no need to make custom-plan
-	 * providers do this
-	 */
-	copy_path_costsize(plan, &best_path->path);
-
-	return plan;
-}
 
 /*****************************************************************************
  *
@@ -2063,6 +2017,53 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
 	return scan_plan;
 }
 
+/*
+ * create_custom_plan
+ *
+ * Transform a CustomPath into a Plan.
+ */
+static Plan *
+create_customscan_plan(PlannerInfo *root, CustomPath *best_path,
+					   List *tlist, List *scan_clauses)
+{
+	Plan	   *plan;
+	RelOptInfo *rel = best_path->path.parent;
+
+	/*
+	 * Right now, all we can support is CustomScan node which is associated
+	 * with a particular base relation to be scanned.
+	 */
+	Assert(rel && rel->reloptkind == RELOPT_BASEREL);
+
+	/*
+	 * Sort clauses into the best execution order, although custom-scan
+	 * provider can reorder them again.
+	 */
+	scan_clauses = order_qual_clauses(root, scan_clauses);
+
+	/*
+	 * Invoke custom plan provider to create the Plan node represented by the
+	 * CustomPath.
+	 */
+	plan = best_path->methods->PlanCustomPath(root, rel, best_path, tlist,
+											  scan_clauses);
+
+	/*
+	 * NOTE: unlike create_foreignscan_plan(), it is the responsibility of the
+	 * custom plan provider to replace outer-relation variables with nestloop
+	 * params, because we cannot know what expression trees may be held in
+	 * private fields.
+	 */
+
+	/*
+	 * Copy cost data from Path to Plan; no need to make custom-plan providers
+	 * do this
+	 */
+	copy_path_costsize(plan, &best_path->path);
+
+	return plan;
+}
+
 
 /*****************************************************************************
  *
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index bbc68a05a6c..e42972750b9 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -587,12 +587,13 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
 					fix_scan_list(root, cscan->scan.plan.targetlist, rtoffset);
 				cscan->scan.plan.qual =
 					fix_scan_list(root, cscan->scan.plan.qual, rtoffset);
+
 				/*
-				 * The core implementation applies the routine to fixup
-				 * varno on the target-list and scan qualifier.
-				 * If custom-scan has additional expression nodes on its
-				 * private fields, it has to apply same fixup on them.
-				 * Otherwise, the custom-plan provider can skip this callback.
+				 * The core implementation applies the routine to fixup varno
+				 * on the target-list and scan qualifier. If custom-scan has
+				 * additional expression nodes on its private fields, it has
+				 * to apply same fixup on them. Otherwise, the custom-plan
+				 * provider can skip this callback.
 				 */
 				if (cscan->methods->SetCustomScanRef)
 					cscan->methods->SetCustomScanRef(root, cscan, rtoffset);
@@ -1083,7 +1084,7 @@ copyVar(Var *var)
  * We assume it's okay to update opcode info in-place.  So this could possibly
  * scribble on the planner's input data structures, but it's OK.
  */
-void
+static void
 fix_expr_common(PlannerInfo *root, Node *node)
 {
 	/* We assume callers won't call us on a NULL pointer */
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 6f1c6cfb2aa..121b9ff3e45 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1929,10 +1929,10 @@ reparameterize_path(PlannerInfo *root, Path *path,
 }
 
 /*****************************************************************************
- *     creation of custom-plan paths
+ *	   creation of custom-plan paths
  *****************************************************************************/
 
-static List	   *custom_path_providers = NIL;
+static List *custom_path_providers = NIL;
 
 /*
  * register_custom_path_provider
@@ -1942,12 +1942,13 @@ static List	   *custom_path_providers = NIL;
  * methods of scanning a relation.
  */
 void
-register_custom_path_provider(CustomPathMethods *cpp_methods)
+register_custom_path_provider(const CustomPathMethods *cpp_methods)
 {
-	MemoryContext	oldcxt;
+	MemoryContext oldcxt;
 
 	oldcxt = MemoryContextSwitchTo(TopMemoryContext);
-	custom_path_providers = lappend(custom_path_providers, cpp_methods);
+	custom_path_providers = lappend(custom_path_providers,
+									(void *) cpp_methods);
 	MemoryContextSwitchTo(oldcxt);
 }
 
@@ -1963,9 +1964,9 @@ create_customscan_paths(PlannerInfo *root,
 						RelOptInfo *baserel,
 						RangeTblEntry *rte)
 {
-	ListCell	   *cell;
+	ListCell   *cell;
 
-	foreach (cell, custom_path_providers)
+	foreach(cell, custom_path_providers)
 	{
 		const CustomPathMethods *cpp_methods = lfirst(cell);