Assumptions about the number of parallel workers
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
Attachments:
num_workers_checks.patchtext/x-diffDownload
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;
Hi,
On 2020-02-05 10:50:05 +0100, Antonin Houska wrote:
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.
It's probably related to force_parallel_mode. With that we'll IIRC
generate gather nodes even if num_workers == 0.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2020-02-05 10:50:05 +0100, Antonin Houska wrote:
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.It's probably related to force_parallel_mode. With that we'll IIRC
generate gather nodes even if num_workers == 0.
Those Gather nodes still have non-zero num_workers, see this part of
standard_planner:
if (force_parallel_mode != FORCE_PARALLEL_OFF && top_plan->parallel_safe)
{
...
gather->num_workers = 1;
gather->single_copy = true;
Also, if it num_workers was zero for any reason, my patch would probably make
regression tests fail. However I haven't noticed any assertion failure.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
On Wed, Feb 5, 2020 at 4:49 AM Antonin Houska <ah@cybertec.at> wrote:
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.
Hmm. There are some cases where we plan on using a Gather node but
then can't actually fire up parallelism because we run out of DSM
segments or we run out of background workers. But the Gather is just
part of the plan, so it would still have num_workers > 0 in those
cases. This might just have been a thinko on my part, but I'm not
totally sure.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
On 2020-02-07 09:44:34 +0100, Antonin Houska wrote:
Those Gather nodes still have non-zero num_workers, see this part of
standard_planner:if (force_parallel_mode != FORCE_PARALLEL_OFF && top_plan->parallel_safe)
{
...
gather->num_workers = 1;
gather->single_copy = true;
Ick. Looks like you might be right...
Also, if it num_workers was zero for any reason, my patch would probably make
regression tests fail. However I haven't noticed any assertion failure.
That however, is not at all guaranteed. The regression tests don't run
(or at least not much) with force_parallel_mode set. You can try
yourself though, with something like
PGOPTIONS='-c force_parallel_mode=regress' make check
Greetings,
Andres Freund