Skip to content

Commit a34fa8e

Browse files
tglsfdctglsfdc
committed
Initial code review for CustomScan patch.
Get rid of the pernicious entanglement between planner and executor headers introduced by commit 0b03e59. 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.
1 parent 081a604 commit a34fa8e

File tree

11 files changed

+164
-156
lines changed

11 files changed

+164
-156
lines changed

‎src/backend/executor/execAmi.c

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -381,11 +381,14 @@ ExecRestrPos(PlanState *node)
381381
}
382382

383383
/*
384-
* ExecSupportsMarkRestore - does a plan type support mark/restore?
384+
* ExecSupportsMarkRestore - does a Path support mark/restore?
385+
*
386+
* This is used during planning and so must accept a Path, not a Plan.
387+
* We keep it here to be adjacent to the routines above, which also must
388+
* know which plan types support mark/restore.
385389
*
386390
* XXX Ideally, all plan node types would support mark/restore, and this
387391
* wouldn't be needed. For now, this had better match the routines above.
388-
* But note the test is on Plan nodetype, not PlanState nodetype.
389392
*
390393
* (However, since the only present use of mark/restore is in mergejoin,
391394
* there is no need to support mark/restore in any plan type that is not
@@ -395,6 +398,11 @@ ExecRestrPos(PlanState *node)
395398
bool
396399
ExecSupportsMarkRestore(Path *pathnode)
397400
{
401+
/*
402+
* For consistency with the routines above, we do not examine the nodeTag
403+
* but rather the pathtype, which is the Plan node type the Path would
404+
* produce.
405+
*/
398406
switch (pathnode->pathtype)
399407
{
400408
case T_SeqScan:
@@ -406,27 +414,23 @@ ExecSupportsMarkRestore(Path *pathnode)
406414
case T_Sort:
407415
return true;
408416

417+
case T_CustomScan:
418+
Assert(IsA(pathnode, CustomPath));
419+
if (((CustomPath *) pathnode)->flags & CUSTOMPATH_SUPPORT_MARK_RESTORE)
420+
return true;
421+
return false;
422+
409423
case T_Result:
410424

411425
/*
412-
* T_Result only supports mark/restore if it has a child plan that
413-
* does, so we do not have enough information to give a really
414-
* correct answer. However, for current uses it's enough to
415-
* always say "false", because this routine is not asked about
416-
* gating Result plans, only base-case Results.
426+
* Although Result supports mark/restore if it has a child plan
427+
* that does, we presently come here only for ResultPath nodes,
428+
* which represent Result plans without a child plan. So there is
429+
* nothing to recurse to and we can just say "false".
417430
*/
431+
Assert(IsA(pathnode, ResultPath));
418432
return false;
419433

420-
case T_CustomScan:
421-
{
422-
CustomPath *cpath = (CustomPath *) pathnode;
423-
424-
Assert(IsA(cpath, CustomPath));
425-
if (cpath->flags & CUSTOMPATH_SUPPORT_MARK_RESTORE)
426-
return true;
427-
}
428-
break;
429-
430434
default:
431435
break;
432436
}
@@ -491,10 +495,10 @@ ExecSupportsBackwardScan(Plan *node)
491495

492496
case T_CustomScan:
493497
{
494-
uint32 flags = ((CustomScan *) node)->flags;
498+
uint32 flags = ((CustomScan *) node)->flags;
495499

496-
if (TargetListSupportsBackwardScan(node->targetlist) &&
497-
(flags & CUSTOMPATH_SUPPORT_BACKWARD_SCAN) != 0)
500+
if ((flags & CUSTOMPATH_SUPPORT_BACKWARD_SCAN) &&
501+
TargetListSupportsBackwardScan(node->targetlist))
498502
return true;
499503
}
500504
return false;

‎src/backend/optimizer/plan/createplan.c

Lines changed: 49 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,8 @@ static WorkTableScan *create_worktablescan_plan(PlannerInfo *root, Path *best_pa
7878
static ForeignScan *create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
7979
List *tlist, List *scan_clauses);
8080
static Plan *create_customscan_plan(PlannerInfo *root,
81-
CustomPath *best_path,
82-
List *tlist, List *scan_clauses);
81+
CustomPath *best_path,
82+
List *tlist, List *scan_clauses);
8383
static NestLoop *create_nestloop_plan(PlannerInfo *root, NestPath *best_path,
8484
Plan *outer_plan, Plan *inner_plan);
8585
static MergeJoin *create_mergejoin_plan(PlannerInfo *root, MergePath *best_path,
@@ -1083,52 +1083,6 @@ create_unique_plan(PlannerInfo *root, UniquePath *best_path)
10831083
return plan;
10841084
}
10851085

1086-
/*
1087-
* create_custom_plan
1088-
*
1089-
* Transform a CustomPath into a Plan.
1090-
*/
1091-
static Plan *
1092-
create_customscan_plan(PlannerInfo *root, CustomPath *best_path,
1093-
List *tlist, List *scan_clauses)
1094-
{
1095-
Plan *plan;
1096-
RelOptInfo *rel = best_path->path.parent;
1097-
1098-
/*
1099-
* Right now, all we can support is CustomScan node which is associated
1100-
* with a particular base relation to be scanned.
1101-
*/
1102-
Assert(rel && rel->reloptkind == RELOPT_BASEREL);
1103-
1104-
/*
1105-
* Sort clauses into the best execution order, although custom-scan
1106-
* provider can reorder them again.
1107-
*/
1108-
scan_clauses = order_qual_clauses(root, scan_clauses);
1109-
1110-
/*
1111-
* Create a CustomScan (or its inheritance) node according to
1112-
* the supplied CustomPath.
1113-
*/
1114-
plan = best_path->methods->PlanCustomPath(root, rel, best_path, tlist,
1115-
scan_clauses);
1116-
1117-
/*
1118-
* NOTE: unlike create_foreignscan_plan(), it is responsibility of
1119-
* the custom plan provider to replace outer-relation variables
1120-
* with nestloop params, because we cannot know how many expression
1121-
* trees are held in the private fields.
1122-
*/
1123-
1124-
/*
1125-
* Copy cost data from Path to Plan; no need to make custom-plan
1126-
* providers do this
1127-
*/
1128-
copy_path_costsize(plan, &best_path->path);
1129-
1130-
return plan;
1131-
}
11321086

11331087
/*****************************************************************************
11341088
*
@@ -2063,6 +2017,53 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
20632017
return scan_plan;
20642018
}
20652019

2020+
/*
2021+
* create_custom_plan
2022+
*
2023+
* Transform a CustomPath into a Plan.
2024+
*/
2025+
static Plan *
2026+
create_customscan_plan(PlannerInfo *root, CustomPath *best_path,
2027+
List *tlist, List *scan_clauses)
2028+
{
2029+
Plan *plan;
2030+
RelOptInfo *rel = best_path->path.parent;
2031+
2032+
/*
2033+
* Right now, all we can support is CustomScan node which is associated
2034+
* with a particular base relation to be scanned.
2035+
*/
2036+
Assert(rel && rel->reloptkind == RELOPT_BASEREL);
2037+
2038+
/*
2039+
* Sort clauses into the best execution order, although custom-scan
2040+
* provider can reorder them again.
2041+
*/
2042+
scan_clauses = order_qual_clauses(root, scan_clauses);
2043+
2044+
/*
2045+
* Invoke custom plan provider to create the Plan node represented by the
2046+
* CustomPath.
2047+
*/
2048+
plan = best_path->methods->PlanCustomPath(root, rel, best_path, tlist,
2049+
scan_clauses);
2050+
2051+
/*
2052+
* NOTE: unlike create_foreignscan_plan(), it is the responsibility of the
2053+
* custom plan provider to replace outer-relation variables with nestloop
2054+
* params, because we cannot know what expression trees may be held in
2055+
* private fields.
2056+
*/
2057+
2058+
/*
2059+
* Copy cost data from Path to Plan; no need to make custom-plan providers
2060+
* do this
2061+
*/
2062+
copy_path_costsize(plan, &best_path->path);
2063+
2064+
return plan;
2065+
}
2066+
20662067

20672068
/*****************************************************************************
20682069
*

‎src/backend/optimizer/plan/setrefs.c

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -587,12 +587,13 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
587587
fix_scan_list(root, cscan->scan.plan.targetlist, rtoffset);
588588
cscan->scan.plan.qual =
589589
fix_scan_list(root, cscan->scan.plan.qual, rtoffset);
590+
590591
/*
591-
* The core implementation applies the routine to fixup
592-
* varno on the target-list and scan qualifier.
593-
* If custom-scan has additional expression nodes on its
594-
* private fields, it has to apply same fixup on them.
595-
* Otherwise, the custom-plan provider can skip this callback.
592+
* The core implementation applies the routine to fixup varno
593+
* on the target-list and scan qualifier. If custom-scan has
594+
* additional expression nodes on its private fields, it has
595+
* to apply same fixup on them. Otherwise, the custom-plan
596+
* provider can skip this callback.
596597
*/
597598
if (cscan->methods->SetCustomScanRef)
598599
cscan->methods->SetCustomScanRef(root, cscan, rtoffset);
@@ -1083,7 +1084,7 @@ copyVar(Var *var)
10831084
* We assume it's okay to update opcode info in-place. So this could possibly
10841085
* scribble on the planner's input data structures, but it's OK.
10851086
*/
1086-
void
1087+
static void
10871088
fix_expr_common(PlannerInfo *root, Node *node)
10881089
{
10891090
/* We assume callers won't call us on a NULL pointer */

‎src/backend/optimizer/util/pathnode.c

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1929,10 +1929,10 @@ reparameterize_path(PlannerInfo *root, Path *path,
19291929
}
19301930

19311931
/*****************************************************************************
1932-
* creation of custom-plan paths
1932+
* creation of custom-plan paths
19331933
*****************************************************************************/
19341934

1935-
static List *custom_path_providers = NIL;
1935+
static List *custom_path_providers = NIL;
19361936

19371937
/*
19381938
* register_custom_path_provider
@@ -1942,12 +1942,13 @@ static List *custom_path_providers = NIL;
19421942
* methods of scanning a relation.
19431943
*/
19441944
void
1945-
register_custom_path_provider(CustomPathMethods *cpp_methods)
1945+
register_custom_path_provider(const CustomPathMethods *cpp_methods)
19461946
{
1947-
MemoryContext oldcxt;
1947+
MemoryContext oldcxt;
19481948

19491949
oldcxt = MemoryContextSwitchTo(TopMemoryContext);
1950-
custom_path_providers = lappend(custom_path_providers, cpp_methods);
1950+
custom_path_providers = lappend(custom_path_providers,
1951+
(void *) cpp_methods);
19511952
MemoryContextSwitchTo(oldcxt);
19521953
}
19531954

@@ -1963,9 +1964,9 @@ create_customscan_paths(PlannerInfo *root,
19631964
RelOptInfo *baserel,
19641965
RangeTblEntry *rte)
19651966
{
1966-
ListCell *cell;
1967+
ListCell *cell;
19671968

1968-
foreach (cell, custom_path_providers)
1969+
foreach(cell, custom_path_providers)
19691970
{
19701971
const CustomPathMethods *cpp_methods = lfirst(cell);
19711972

‎src/include/executor/executor.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
#include "executor/execdesc.h"
1818
#include "nodes/parsenodes.h"
19-
#include "nodes/relation.h"
2019
#include "utils/lockwaitpolicy.h"
2120

2221

@@ -101,10 +100,12 @@ extern PGDLLIMPORT ExecutorCheckPerms_hook_type ExecutorCheckPerms_hook;
101100
/*
102101
* prototypes from functions in execAmi.c
103102
*/
103+
struct Path; /* avoid including relation.h here */
104+
104105
extern void ExecReScan(PlanState *node);
105106
extern void ExecMarkPos(PlanState *node);
106107
extern void ExecRestrPos(PlanState *node);
107-
extern bool ExecSupportsMarkRestore(Path *pathnode);
108+
extern bool ExecSupportsMarkRestore(struct Path *pathnode);
108109
extern bool ExecSupportsBackwardScan(Plan *node);
109110
extern bool ExecMaterializesOutput(NodeTag plantype);
110111

‎src/include/executor/nodeCustom.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,14 @@
1111
*/
1212
#ifndef NODECUSTOM_H
1313
#define NODECUSTOM_H
14-
#include "nodes/plannodes.h"
14+
1515
#include "nodes/execnodes.h"
1616

1717
/*
1818
* General executor code
1919
*/
2020
extern CustomScanState *ExecInitCustomScan(CustomScan *custom_scan,
21-
EState *estate, int eflags);
21+
EState *estate, int eflags);
2222
extern TupleTableSlot *ExecCustomScan(CustomScanState *node);
2323
extern Node *MultiExecCustomScan(CustomScanState *node);
2424
extern void ExecEndCustomScan(CustomScanState *node);
@@ -27,4 +27,4 @@ extern void ExecReScanCustomScan(CustomScanState *node);
2727
extern void ExecCustomMarkPos(CustomScanState *node);
2828
extern void ExecCustomRestrPos(CustomScanState *node);
2929

30-
#endif /* NODECUSTOM_H */
30+
#endif /* NODECUSTOM_H */

‎src/include/nodes/execnodes.h

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
#include "executor/instrument.h"
2020
#include "nodes/params.h"
2121
#include "nodes/plannodes.h"
22-
#include "nodes/relation.h"
2322
#include "utils/reltrigger.h"
2423
#include "utils/sortsupport.h"
2524
#include "utils/tuplestore.h"
@@ -1512,39 +1511,39 @@ typedef struct ForeignScanState
15121511
* CustomScan nodes are used to execute custom code within executor.
15131512
* ----------------
15141513
*/
1515-
struct CustomExecMethods;
1516-
struct ExplainState; /* to avoid to include explain.h here */
1517-
1518-
typedef struct CustomScanState
1519-
{
1520-
ScanState ss;
1521-
uint32 flags; /* mask of CUSTOMPATH_* flags defined in relation.h*/
1522-
const struct CustomExecMethods *methods;
1523-
} CustomScanState;
1514+
struct ExplainState; /* avoid including explain.h here */
1515+
struct CustomScanState;
15241516

15251517
typedef struct CustomExecMethods
15261518
{
1527-
const char *CustomName;
1519+
const char *CustomName;
15281520

15291521
/* EXECUTOR methods */
1530-
void (*BeginCustomScan)(CustomScanState *node,
1531-
EState *estate,
1532-
int eflags);
1533-
TupleTableSlot *(*ExecCustomScan)(CustomScanState *node);
1534-
void (*EndCustomScan)(CustomScanState *node);
1535-
void (*ReScanCustomScan)(CustomScanState *node);
1536-
void (*MarkPosCustomScan)(CustomScanState *node);
1537-
void (*RestrPosCustomScan)(CustomScanState *node);
1522+
void (*BeginCustomScan) (struct CustomScanState *node,
1523+
EState *estate,
1524+
int eflags);
1525+
TupleTableSlot *(*ExecCustomScan) (struct CustomScanState *node);
1526+
void (*EndCustomScan) (struct CustomScanState *node);
1527+
void (*ReScanCustomScan) (struct CustomScanState *node);
1528+
void (*MarkPosCustomScan) (struct CustomScanState *node);
1529+
void (*RestrPosCustomScan) (struct CustomScanState *node);
15381530

15391531
/* EXPLAIN support */
1540-
void (*ExplainCustomScan)(CustomScanState *node,
1541-
List *ancestors,
1542-
struct ExplainState *es);
1543-
Node *(*GetSpecialCustomVar)(CustomScanState *node,
1544-
Var *varnode,
1545-
PlanState **child_ps);
1532+
void (*ExplainCustomScan) (struct CustomScanState *node,
1533+
List *ancestors,
1534+
struct ExplainState *es);
1535+
Node *(*GetSpecialCustomVar) (struct CustomScanState *node,
1536+
Var *varnode,
1537+
PlanState **child_ps);
15461538
} CustomExecMethods;
15471539

1540+
typedef struct CustomScanState
1541+
{
1542+
ScanState ss;
1543+
uint32 flags; /* mask of CUSTOMPATH_* flags, see relation.h */
1544+
const CustomExecMethods *methods;
1545+
} CustomScanState;
1546+
15481547
/* ----------------------------------------------------------------
15491548
* Join State Information
15501549
* ----------------------------------------------------------------

0 commit comments

Comments
 (0)