Commit 4ea9948e authored by Tom Lane's avatar Tom Lane

Fix up parallel-safety marking for appendrels.

The previous coding assumed that the value derived by
set_rel_consider_parallel() for an appendrel parent would be accurate for
all the appendrel's children; but this is not so, for example because one
child might scan a temp table.  Instead, apply set_rel_consider_parallel()
to each child rel as well as the parent, and then take the AND of the
results as controlling parallel safety for the appendrel as a whole.

(We might someday be able to deal more intelligently than this with cases
in which some of the childrels are parallel-safe and others not, but that's
for later.)

Robert Haas and Tom Lane
parent 2c6e6471
...@@ -165,8 +165,8 @@ make_one_rel(PlannerInfo *root, List *joinlist) ...@@ -165,8 +165,8 @@ make_one_rel(PlannerInfo *root, List *joinlist)
set_base_rel_consider_startup(root); set_base_rel_consider_startup(root);
/* /*
* Generate access paths for the base rels. set_base_rel_sizes also sets * Compute size estimates and consider_parallel flags for each base rel,
* the consider_parallel flag for each baserel, if appropriate. * then generate access paths.
*/ */
set_base_rel_sizes(root); set_base_rel_sizes(root);
set_base_rel_pathlists(root); set_base_rel_pathlists(root);
...@@ -234,7 +234,7 @@ set_base_rel_consider_startup(PlannerInfo *root) ...@@ -234,7 +234,7 @@ set_base_rel_consider_startup(PlannerInfo *root)
* *
* We do this in a separate pass over the base rels so that rowcount * We do this in a separate pass over the base rels so that rowcount
* estimates are available for parameterized path generation, and also so * estimates are available for parameterized path generation, and also so
* that the consider_parallel flag is set correctly before we begin to * that each rel's consider_parallel flag is set correctly before we begin to
* generate paths. * generate paths.
*/ */
static void static void
...@@ -262,9 +262,10 @@ set_base_rel_sizes(PlannerInfo *root) ...@@ -262,9 +262,10 @@ set_base_rel_sizes(PlannerInfo *root)
/* /*
* If parallelism is allowable for this query in general, see whether * If parallelism is allowable for this query in general, see whether
* it's allowable for this rel in particular. We have to do this * it's allowable for this rel in particular. We have to do this
* before set_rel_size, because that if this is an inheritance parent, * before set_rel_size(), because (a) if this rel is an inheritance
* set_append_rel_size will pass the consider_parallel flag down to * parent, set_append_rel_size() will use and perhaps change the rel's
* inheritance children. * consider_parallel flag, and (b) for some RTE types, set_rel_size()
* goes ahead and makes paths immediately.
*/ */
if (root->glob->parallelModeOK) if (root->glob->parallelModeOK)
set_rel_consider_parallel(root, rel, rte); set_rel_consider_parallel(root, rel, rte);
...@@ -494,18 +495,24 @@ set_plain_rel_size(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte) ...@@ -494,18 +495,24 @@ set_plain_rel_size(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
/* /*
* If this relation could possibly be scanned from within a worker, then set * If this relation could possibly be scanned from within a worker, then set
* the consider_parallel flag. The flag has previously been initialized to * its consider_parallel flag.
* false, so we just bail out if it becomes clear that we can't safely set it.
*/ */
static void static void
set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel, set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel,
RangeTblEntry *rte) RangeTblEntry *rte)
{ {
/*
* The flag has previously been initialized to false, so we can just
* return if it becomes clear that we can't safely set it.
*/
Assert(!rel->consider_parallel);
/* Don't call this if parallelism is disallowed for the entire query. */ /* Don't call this if parallelism is disallowed for the entire query. */
Assert(root->glob->parallelModeOK); Assert(root->glob->parallelModeOK);
/* Don't call this for non-baserels. */ /* This should only be called for baserels and appendrel children. */
Assert(rel->reloptkind == RELOPT_BASEREL); Assert(rel->reloptkind == RELOPT_BASEREL ||
rel->reloptkind == RELOPT_OTHER_MEMBER_REL);
/* Assorted checks based on rtekind. */ /* Assorted checks based on rtekind. */
switch (rte->rtekind) switch (rte->rtekind)
...@@ -556,6 +563,13 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel, ...@@ -556,6 +563,13 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel,
if (!rel->fdwroutine->IsForeignScanParallelSafe(root, rel, rte)) if (!rel->fdwroutine->IsForeignScanParallelSafe(root, rel, rte))
return; return;
} }
/*
* There are additional considerations for appendrels, which we'll
* deal with in set_append_rel_size and set_append_rel_pathlist.
* For now, just set consider_parallel based on the rel's own
* quals and targetlist.
*/
break; break;
case RTE_SUBQUERY: case RTE_SUBQUERY:
...@@ -607,8 +621,9 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel, ...@@ -607,8 +621,9 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel,
* give up on parallelizing access to this relation. We could consider * give up on parallelizing access to this relation. We could consider
* instead postponing application of the restricted quals until we're * instead postponing application of the restricted quals until we're
* above all the parallelism in the plan tree, but it's not clear that * above all the parallelism in the plan tree, but it's not clear that
* this would be a win in very many cases, and it might be tricky to make * that would be a win in very many cases, and it might be tricky to make
* outer join clauses work correctly. * outer join clauses work correctly. It would likely break equivalence
* classes, too.
*/ */
if (has_parallel_hazard((Node *) rel->baserestrictinfo, false)) if (has_parallel_hazard((Node *) rel->baserestrictinfo, false))
return; return;
...@@ -965,9 +980,6 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, ...@@ -965,9 +980,6 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
continue; continue;
} }
/* Copy consider_parallel flag from parent. */
childrel->consider_parallel = rel->consider_parallel;
/* /*
* CE failed, so finish copying/modifying targetlist and join quals. * CE failed, so finish copying/modifying targetlist and join quals.
* *
...@@ -1007,6 +1019,16 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, ...@@ -1007,6 +1019,16 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
* otherrels. So we just leave the child's attr_needed empty. * otherrels. So we just leave the child's attr_needed empty.
*/ */
/*
* If parallelism is allowable for this query in general, see whether
* it's allowable for this childrel in particular. But if we've
* already decided the appendrel is not parallel-safe as a whole,
* there's no point in considering parallelism for this child. For
* consistency, do this before calling set_rel_size() for the child.
*/
if (root->glob->parallelModeOK && rel->consider_parallel)
set_rel_consider_parallel(root, childrel, childRTE);
/* /*
* Compute the child's size. * Compute the child's size.
*/ */
...@@ -1023,6 +1045,18 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, ...@@ -1023,6 +1045,18 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
/* We have at least one live child. */ /* We have at least one live child. */
has_live_children = true; has_live_children = true;
/*
* If any live child is not parallel-safe, treat the whole appendrel
* as not parallel-safe. In future we might be able to generate plans
* in which some children are farmed out to workers while others are
* not; but we don't have that today, so it's a waste to consider
* partial paths anywhere in the appendrel unless it's all safe.
* (Child rels visited before this one will be unmarked in
* set_append_rel_pathlist().)
*/
if (!childrel->consider_parallel)
rel->consider_parallel = false;
/* /*
* Accumulate size information from each live child. * Accumulate size information from each live child.
*/ */
...@@ -1139,6 +1173,15 @@ set_append_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, ...@@ -1139,6 +1173,15 @@ set_append_rel_pathlist(PlannerInfo *root, RelOptInfo *rel,
childRTE = root->simple_rte_array[childRTindex]; childRTE = root->simple_rte_array[childRTindex];
childrel = root->simple_rel_array[childRTindex]; childrel = root->simple_rel_array[childRTindex];
/*
* If set_append_rel_size() decided the parent appendrel was
* parallel-unsafe at some point after visiting this child rel, we
* need to propagate the unsafety marking down to the child, so that
* we don't generate useless partial paths for it.
*/
if (!rel->consider_parallel)
childrel->consider_parallel = false;
/* /*
* Compute the child's access paths. * Compute the child's access paths.
*/ */
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment