I can't figure out why ExecGather/ExecGatherMerge do check whether num_workers
is non-zero. I think the code would be a bit clearer if these tests were
replaced with Assert() statements, as the attached patch does.
In addition, if my assumptions are correct, I think that only Gather node
needs the single_copy field, but GatherPath does not.
In the patch I also added Assert() to add_partial_path so that I'm more likely
to catch special cases. Regression tests passed though.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c
index 6b8ed867d5..09aecf7bfe 100644
--- a/src/backend/executor/nodeGather.c
+++ b/src/backend/executor/nodeGather.c
@@ -158,11 +158,17 @@ ExecGather(PlanState *pstate)
EState *estate = node->ps.state;
Gather *gather = (Gather *) node->ps.plan;
+ /*
+ * Gather node should not be created if there are no parallel
+ * workers.
+ */
+ Assert(gather->num_workers > 0);
+
/*
* Sometimes we might have to run without parallelism; but if parallel
* mode is active then we can try to fire up some workers.
*/
- if (gather->num_workers > 0 && estate->es_use_parallel_mode)
+ if (estate->es_use_parallel_mode)
{
ParallelContext *pcxt;
diff --git a/src/backend/executor/nodeGatherMerge.c b/src/backend/executor/nodeGatherMerge.c
index 317ddb4ae2..80c3f1561b 100644
--- a/src/backend/executor/nodeGatherMerge.c
+++ b/src/backend/executor/nodeGatherMerge.c
@@ -202,11 +202,17 @@ ExecGatherMerge(PlanState *pstate)
EState *estate = node->ps.state;
GatherMerge *gm = castNode(GatherMerge, node->ps.plan);
+ /*
+ * GatherMerge node should not be created if there are no parallel
+ * workers.
+ */
+ Assert(gm->num_workers > 0);
+
/*
* Sometimes we might have to run without parallelism; but if parallel
* mode is active then we can try to fire up some workers.
*/
- if (gm->num_workers > 0 && estate->es_use_parallel_mode)
+ if (estate->es_use_parallel_mode)
{
ParallelContext *pcxt;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index d76fae44b8..a1fdd3afef 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1907,7 +1907,6 @@ _outGatherPath(StringInfo str, const GatherPath *node)
_outPathInfo(str, (const Path *) node);
WRITE_NODE_FIELD(subpath);
- WRITE_BOOL_FIELD(single_copy);
WRITE_INT_FIELD(num_workers);
}
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index e048d200bb..ee3b8f7fd5 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -276,7 +276,7 @@ static Unique *make_unique_from_sortclauses(Plan *lefttree, List *distinctList);
static Unique *make_unique_from_pathkeys(Plan *lefttree,
List *pathkeys, int numCols);
static Gather *make_gather(List *qptlist, List *qpqual,
- int nworkers, int rescan_param, bool single_copy, Plan *subplan);
+ int nworkers, int rescan_param, Plan *subplan);
static SetOp *make_setop(SetOpCmd cmd, SetOpStrategy strategy, Plan *lefttree,
List *distinctList, AttrNumber flagColIdx, int firstFlag,
long numGroups);
@@ -1727,7 +1727,6 @@ create_gather_plan(PlannerInfo *root, GatherPath *best_path)
NIL,
best_path->num_workers,
assign_special_exec_param(root),
- best_path->single_copy,
subplan);
copy_generic_path_info(&gather_plan->plan, &best_path->path);
@@ -6450,7 +6449,6 @@ make_gather(List *qptlist,
List *qpqual,
int nworkers,
int rescan_param,
- bool single_copy,
Plan *subplan)
{
Gather *node = makeNode(Gather);
@@ -6462,7 +6460,7 @@ make_gather(List *qptlist,
plan->righttree = NULL;
node->num_workers = nworkers;
node->rescan_param = rescan_param;
- node->single_copy = single_copy;
+ node->single_copy = false;
node->invisible = false;
node->initParam = NULL;
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index e6d08aede5..31316cd2ec 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -758,6 +758,12 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path)
/* Path to be added must be parallel safe. */
Assert(new_path->parallel_safe);
+ /*
+ * No partial path should be created if there's not enough work for
+ * parallel workers.
+ */
+ Assert(new_path->parallel_workers > 0);
+
/* Relation should be OK for parallelism, too. */
Assert(parent_rel->consider_parallel);
@@ -1847,6 +1853,7 @@ create_gather_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath,
GatherPath *pathnode = makeNode(GatherPath);
Assert(subpath->parallel_safe);
+ Assert(subpath->parallel_workers > 0);
pathnode->path.pathtype = T_Gather;
pathnode->path.parent = rel;
@@ -1860,14 +1867,6 @@ create_gather_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath,
pathnode->subpath = subpath;
pathnode->num_workers = subpath->parallel_workers;
- pathnode->single_copy = false;
-
- if (pathnode->num_workers == 0)
- {
- pathnode->path.pathkeys = subpath->pathkeys;
- pathnode->num_workers = 1;
- pathnode->single_copy = true;
- }
cost_gather(pathnode, root, rel, pathnode->path.param_info, rows);
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 3d3be197e0..3e81045b53 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -1458,14 +1458,12 @@ typedef struct UniquePath
/*
* GatherPath runs several copies of a plan in parallel and collects the
- * results. The parallel leader may also execute the plan, unless the
- * single_copy flag is set.
+ * results. The parallel leader may also execute the plan.
*/
typedef struct GatherPath
{
Path path;
Path *subpath; /* path for each worker */
- bool single_copy; /* don't execute path more than once */
int num_workers; /* number of workers sought to help */
} GatherPath;
