Initial code review for CustomScan patch.
authorTom Lane <[email protected]>
Thu, 20 Nov 2014 23:36:07 +0000 (18:36 -0500)
committerTom Lane <[email protected]>
Thu, 20 Nov 2014 23:36:07 +0000 (18:36 -0500)
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.

src/backend/executor/execAmi.c
src/backend/optimizer/plan/createplan.c
src/backend/optimizer/plan/setrefs.c
src/backend/optimizer/util/pathnode.c
src/include/executor/executor.h
src/include/executor/nodeCustom.h
src/include/nodes/execnodes.h
src/include/nodes/plannodes.h
src/include/nodes/relation.h
src/include/optimizer/pathnode.h
src/include/optimizer/planmain.h

index b14e08cd1af5b72d292f9fdeda1b36fac3a95175..643aaace3a9c8372223ae30ab4e5d7a0fb902dbd 100644 (file)
@@ -381,11 +381,14 @@ ExecRestrPos(PlanState *node)
 }
 
 /*
- * ExecSupportsMarkRestore - does a plan type support mark/restore?
+ * ExecSupportsMarkRestore - does a Path support mark/restore?
+ *
+ * This is used during planning and so must accept a Path, not a Plan.
+ * We keep it here to be adjacent to the routines above, which also must
+ * know which plan types support mark/restore.
  *
  * XXX Ideally, all plan node types would support mark/restore, and this
  * wouldn't be needed.  For now, this had better match the routines above.
- * But note the test is on Plan nodetype, not PlanState nodetype.
  *
  * (However, since the only present use of mark/restore is in mergejoin,
  * there is no need to support mark/restore in any plan type that is not
@@ -395,6 +398,11 @@ ExecRestrPos(PlanState *node)
 bool
 ExecSupportsMarkRestore(Path *pathnode)
 {
+   /*
+    * For consistency with the routines above, we do not examine the nodeTag
+    * but rather the pathtype, which is the Plan node type the Path would
+    * produce.
+    */
    switch (pathnode->pathtype)
    {
        case T_SeqScan:
@@ -406,27 +414,23 @@ ExecSupportsMarkRestore(Path *pathnode)
        case T_Sort:
            return true;
 
+       case T_CustomScan:
+           Assert(IsA(pathnode, CustomPath));
+           if (((CustomPath *) pathnode)->flags & CUSTOMPATH_SUPPORT_MARK_RESTORE)
+               return true;
+           return false;
+
        case T_Result:
 
            /*
-            * T_Result only supports mark/restore if it has a child plan that
-            * does, so we do not have enough information to give a really
-            * correct answer.  However, for current uses it's enough to
-            * always say "false", because this routine is not asked about
-            * gating Result plans, only base-case Results.
+            * Although Result supports mark/restore if it has a child plan
+            * that does, we presently come here only for ResultPath nodes,
+            * which represent Result plans without a child plan.  So there is
+            * nothing to recurse to and we can just say "false".
             */
+           Assert(IsA(pathnode, ResultPath));
            return false;
 
-       case T_CustomScan:
-           {
-               CustomPath *cpath = (CustomPath *) pathnode;
-
-               Assert(IsA(cpath, CustomPath));
-               if (cpath->flags & CUSTOMPATH_SUPPORT_MARK_RESTORE)
-                   return true;
-           }
-           break;
-
        default:
            break;
    }
@@ -491,10 +495,10 @@ ExecSupportsBackwardScan(Plan *node)
 
        case T_CustomScan:
            {
-               uint32  flags = ((CustomScan *) node)->flags;
+               uint32      flags = ((CustomScan *) node)->flags;
 
-               if (TargetListSupportsBackwardScan(node->targetlist) &&
-                   (flags & CUSTOMPATH_SUPPORT_BACKWARD_SCAN) != 0)
+               if ((flags & CUSTOMPATH_SUPPORT_BACKWARD_SCAN) &&
+                   TargetListSupportsBackwardScan(node->targetlist))
                    return true;
            }
            return false;
index 0a85cd990618a113f6d14047eef2bf38f3fc773b..e6bd4c326ca883c8ddb5dbdae964d19d33871af3 100644 (file)
@@ -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;
+}
+
 
 /*****************************************************************************
  *
index bbc68a05a6c7617ccfe2ddf0e16f59332e49b284..e42972750b9a12a8d57950a82c87e89901bb4dbd 100644 (file)
@@ -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 */
index 6f1c6cfb2aaedefa27f84bac5d854baf88c0f53b..121b9ff3e450a2d651c996de80929c4c6430e13c 100644 (file)
@@ -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);
 
index f1b65b4d05039e7511ac1e884d01ce54db57e31b..ed3ae39b66dfb0e488b78b3a0bc524ac696fb0dc 100644 (file)
@@ -16,7 +16,6 @@
 
 #include "executor/execdesc.h"
 #include "nodes/parsenodes.h"
-#include "nodes/relation.h"
 #include "utils/lockwaitpolicy.h"
 
 
@@ -101,10 +100,12 @@ extern PGDLLIMPORT ExecutorCheckPerms_hook_type ExecutorCheckPerms_hook;
 /*
  * prototypes from functions in execAmi.c
  */
+struct Path;                   /* avoid including relation.h here */
+
 extern void ExecReScan(PlanState *node);
 extern void ExecMarkPos(PlanState *node);
 extern void ExecRestrPos(PlanState *node);
-extern bool ExecSupportsMarkRestore(Path *pathnode);
+extern bool ExecSupportsMarkRestore(struct Path *pathnode);
 extern bool ExecSupportsBackwardScan(Plan *node);
 extern bool ExecMaterializesOutput(NodeTag plantype);
 
index 1736d48bfafbb12fb2117749fc79768ac0ca3042..e6f125544bc96f443eebd5cfc0e7c0db739cbb09 100644 (file)
  */
 #ifndef NODECUSTOM_H
 #define NODECUSTOM_H
-#include "nodes/plannodes.h"
+
 #include "nodes/execnodes.h"
 
 /*
  * General executor code
  */
 extern CustomScanState *ExecInitCustomScan(CustomScan *custom_scan,
-                                          EState *estate, int eflags);
+                  EState *estate, int eflags);
 extern TupleTableSlot *ExecCustomScan(CustomScanState *node);
 extern Node *MultiExecCustomScan(CustomScanState *node);
 extern void ExecEndCustomScan(CustomScanState *node);
@@ -27,4 +27,4 @@ extern void ExecReScanCustomScan(CustomScanState *node);
 extern void ExecCustomMarkPos(CustomScanState *node);
 extern void ExecCustomRestrPos(CustomScanState *node);
 
-#endif /* NODECUSTOM_H */
+#endif   /* NODECUSTOM_H */
index 8c8c01f1cd20eb85cbbd4510d46f6c29fd097948..40fb8243ab426024653e44a7b730a77bb295fa8d 100644 (file)
@@ -19,7 +19,6 @@
 #include "executor/instrument.h"
 #include "nodes/params.h"
 #include "nodes/plannodes.h"
-#include "nodes/relation.h"
 #include "utils/reltrigger.h"
 #include "utils/sortsupport.h"
 #include "utils/tuplestore.h"
@@ -1512,39 +1511,39 @@ typedef struct ForeignScanState
  *     CustomScan nodes are used to execute custom code within executor.
  * ----------------
  */
-struct CustomExecMethods;
-struct ExplainState;   /* to avoid to include explain.h here */
-
-typedef struct CustomScanState
-{
-   ScanState   ss;
-   uint32      flags;  /* mask of CUSTOMPATH_* flags defined in relation.h*/
-   const struct CustomExecMethods *methods;
-} CustomScanState;
+struct ExplainState;           /* avoid including explain.h here */
+struct CustomScanState;
 
 typedef struct CustomExecMethods
 {
-   const char     *CustomName;
+   const char *CustomName;
 
    /* EXECUTOR methods */
-   void    (*BeginCustomScan)(CustomScanState *node,
-                              EState *estate,
-                              int eflags);
-   TupleTableSlot *(*ExecCustomScan)(CustomScanState *node);
-   void    (*EndCustomScan)(CustomScanState *node);
-   void    (*ReScanCustomScan)(CustomScanState *node);
-   void    (*MarkPosCustomScan)(CustomScanState *node);
-   void    (*RestrPosCustomScan)(CustomScanState *node);
+   void        (*BeginCustomScan) (struct CustomScanState *node,
+                                               EState *estate,
+                                               int eflags);
+   TupleTableSlot *(*ExecCustomScan) (struct CustomScanState *node);
+   void        (*EndCustomScan) (struct CustomScanState *node);
+   void        (*ReScanCustomScan) (struct CustomScanState *node);
+   void        (*MarkPosCustomScan) (struct CustomScanState *node);
+   void        (*RestrPosCustomScan) (struct CustomScanState *node);
 
    /* EXPLAIN support */
-   void    (*ExplainCustomScan)(CustomScanState *node,
-                                List *ancestors,
-                                struct ExplainState *es);
-   Node   *(*GetSpecialCustomVar)(CustomScanState *node,
-                                  Var *varnode,
-                                  PlanState **child_ps);
+   void        (*ExplainCustomScan) (struct CustomScanState *node,
+                                                 List *ancestors,
+                                                 struct ExplainState *es);
+   Node       *(*GetSpecialCustomVar) (struct CustomScanState *node,
+                                                   Var *varnode,
+                                                   PlanState **child_ps);
 } CustomExecMethods;
 
+typedef struct CustomScanState
+{
+   ScanState   ss;
+   uint32      flags;          /* mask of CUSTOMPATH_* flags, see relation.h */
+   const CustomExecMethods *methods;
+} CustomScanState;
+
 /* ----------------------------------------------------------------
  *              Join State Information
  * ----------------------------------------------------------------
index 9dbb91cb90d897248c351adfd1c053f66b196181..dd300b1a191b975e39e97b9ed260174c186c980a 100644 (file)
@@ -18,7 +18,6 @@
 #include "lib/stringinfo.h"
 #include "nodes/bitmapset.h"
 #include "nodes/primnodes.h"
-#include "nodes/relation.h"
 #include "utils/lockwaitpolicy.h"
 
 
@@ -486,33 +485,36 @@ typedef struct ForeignScan
 } ForeignScan;
 
 /* ----------------
- *     CustomScan node
+ *    CustomScan node
  * ----------------
  */
-struct CustomScanMethods;
-
-typedef struct CustomScan
-{
-   Scan        scan;
-   uint32      flags;  /* mask of CUSTOMPATH_* flags defined in relation.h */
-   struct CustomScanMethods *methods;
-} CustomScan;
+struct PlannerInfo;                /* avoid including relation.h here */
+struct CustomScan;
 
 typedef struct CustomScanMethods
 {
    const char *CustomName;
-   void       (*SetCustomScanRef)(struct PlannerInfo *root,
-                                  CustomScan *cscan,
-                                  int rtoffset);
-   void       (*FinalizeCustomScan)(struct PlannerInfo *root,
-                                    CustomScan *cscan,
-                                    bool (*finalize_primnode)(),
-                                    void *finalize_context);
-   Node      *(*CreateCustomScanState)(CustomScan *cscan);
-   void       (*TextOutCustomScan)(StringInfo str, const CustomScan *node);
-   CustomScan *(*CopyCustomScan)(const CustomScan *from);
+
+   void        (*SetCustomScanRef) (struct PlannerInfo *root,
+                                                struct CustomScan *cscan,
+                                                int rtoffset);
+   void        (*FinalizeCustomScan) (struct PlannerInfo *root,
+                                                  struct CustomScan *cscan,
+                                               bool (*finalize_primnode) (),
+                                                  void *finalize_context);
+   Node       *(*CreateCustomScanState) (struct CustomScan *cscan);
+   void        (*TextOutCustomScan) (StringInfo str,
+                                             const struct CustomScan *node);
+   struct CustomScan *(*CopyCustomScan) (const struct CustomScan *from);
 } CustomScanMethods;
 
+typedef struct CustomScan
+{
+   Scan        scan;
+   uint32      flags;          /* mask of CUSTOMPATH_* flags, see relation.h */
+   const CustomScanMethods *methods;
+} CustomScan;
+
 /*
  * ==========
  * Join nodes
@@ -864,7 +866,7 @@ typedef struct PlanRowMark
    Index       prti;           /* range table index of parent relation */
    Index       rowmarkId;      /* unique identifier for resjunk columns */
    RowMarkType markType;       /* see enum above */
-   LockWaitPolicy waitPolicy;  /* NOWAIT and SKIP LOCKED options */
+   LockWaitPolicy waitPolicy;  /* NOWAIT and SKIP LOCKED options */
    bool        isParent;       /* true if this is a "dummy" parent entry */
 } PlanRowMark;
 
index 05cfbcd2aa158eb1056095c1c712cc3aa566f7ab..7953bf7ed6c808e4dc574fcbcebd1c1f0e5d5c0f 100644 (file)
@@ -897,34 +897,34 @@ typedef struct ForeignPath
  * the structure declared here; providers are expected to make it the first
  * element in a larger structure.
  */
-
-struct CustomPathMethods;
-struct Plan;       /* not to include plannodes.h here */
+struct CustomPath;
 
 #define CUSTOMPATH_SUPPORT_BACKWARD_SCAN   0x0001
 #define CUSTOMPATH_SUPPORT_MARK_RESTORE        0x0002
 
-typedef struct CustomPath
-{
-   Path        path;
-   uint32      flags;
-   const struct CustomPathMethods *methods;
-} CustomPath;
-
 typedef struct CustomPathMethods
 {
    const char *CustomName;
-   void    (*CreateCustomScanPath)(PlannerInfo *root,
-                                   RelOptInfo *baserel,
-                                   RangeTblEntry *rte);
-   struct Plan *(*PlanCustomPath)(PlannerInfo *root,
-                                  RelOptInfo *rel,
-                                  CustomPath *best_path,
-                                  List *tlist,
-                                  List *clauses);
-   void    (*TextOutCustomPath)(StringInfo str, const CustomPath *node);
+
+   void        (*CreateCustomScanPath) (PlannerInfo *root,
+                                                    RelOptInfo *baserel,
+                                                    RangeTblEntry *rte);
+   struct Plan *(*PlanCustomPath) (PlannerInfo *root,
+                                               RelOptInfo *rel,
+                                               struct CustomPath *best_path,
+                                               List *tlist,
+                                               List *clauses);
+   void        (*TextOutCustomPath) (StringInfo str,
+                                             const struct CustomPath *node);
 } CustomPathMethods;
 
+typedef struct CustomPath
+{
+   Path        path;
+   uint32      flags;          /* mask of CUSTOMPATH_* flags, see above */
+   const CustomPathMethods *methods;
+} CustomPath;
+
 /*
  * AppendPath represents an Append plan, ie, successive execution of
  * several member plans.
index 2b67ae6187b61299c9b9bb1ef2de4a22bd2d23f5..0f2882986c0c36d980ccf5833939df9401b8d29d 100644 (file)
@@ -131,11 +131,11 @@ extern Path *reparameterize_path(PlannerInfo *root, Path *path,
 /*
  * Interface definition of custom-scan providers
  */
-extern void register_custom_path_provider(CustomPathMethods *cpp_methods);
+extern void register_custom_path_provider(const CustomPathMethods *cpp_methods);
 
 extern void create_customscan_paths(PlannerInfo *root,
-                                   RelOptInfo *baserel,
-                                   RangeTblEntry *rte);
+                       RelOptInfo *baserel,
+                       RangeTblEntry *rte);
 
 /*
  * prototypes for relnode.c
index c97c5777a07f7d6811d78b1ec19faec46b9a9de9..d6dae8e738e98ff9c47ec98673d159138729f954 100644 (file)
@@ -130,9 +130,8 @@ extern bool query_is_distinct_for(Query *query, List *colnos, List *opids);
  * prototypes for plan/setrefs.c
  */
 extern Plan *set_plan_references(PlannerInfo *root, Plan *plan);
-extern void fix_opfuncids(Node *node);
 extern Node *fix_scan_expr(PlannerInfo *root, Node *node, int rtoffset);
-extern void fix_expr_common(PlannerInfo *root, Node *node);
+extern void fix_opfuncids(Node *node);
 extern void set_opfuncid(OpExpr *opexpr);
 extern void set_sa_opfuncid(ScalarArrayOpExpr *opexpr);
 extern void record_plan_function_dependency(PlannerInfo *root, Oid funcid);