From 4ea9948e58a57316330acb1701f979bc1e872f50 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 3 Jul 2016 17:57:28 -0400 Subject: [PATCH] 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 --- src/backend/optimizer/path/allpaths.c | 73 +++++++++++++++++++++------ 1 file changed, 58 insertions(+), 15 deletions(-) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 535d2e69b2..69c7cf64d5 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -165,8 +165,8 @@ make_one_rel(PlannerInfo *root, List *joinlist) set_base_rel_consider_startup(root); /* - * Generate access paths for the base rels. set_base_rel_sizes also sets - * the consider_parallel flag for each baserel, if appropriate. + * Compute size estimates and consider_parallel flags for each base rel, + * then generate access paths. */ set_base_rel_sizes(root); set_base_rel_pathlists(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 * 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. */ static void @@ -262,9 +262,10 @@ set_base_rel_sizes(PlannerInfo *root) /* * If parallelism is allowable for this query in general, see whether * 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, - * set_append_rel_size will pass the consider_parallel flag down to - * inheritance children. + * before set_rel_size(), because (a) if this rel is an inheritance + * parent, set_append_rel_size() will use and perhaps change the rel's + * consider_parallel flag, and (b) for some RTE types, set_rel_size() + * goes ahead and makes paths immediately. */ if (root->glob->parallelModeOK) set_rel_consider_parallel(root, rel, 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 - * the consider_parallel flag. The flag has previously been initialized to - * false, so we just bail out if it becomes clear that we can't safely set it. + * its consider_parallel flag. */ static void set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel, 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. */ Assert(root->glob->parallelModeOK); - /* Don't call this for non-baserels. */ - Assert(rel->reloptkind == RELOPT_BASEREL); + /* This should only be called for baserels and appendrel children. */ + Assert(rel->reloptkind == RELOPT_BASEREL || + rel->reloptkind == RELOPT_OTHER_MEMBER_REL); /* Assorted checks based on rtekind. */ switch (rte->rtekind) @@ -556,6 +563,13 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel, if (!rel->fdwroutine->IsForeignScanParallelSafe(root, rel, rte)) 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; case RTE_SUBQUERY: @@ -607,8 +621,9 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel, * give up on parallelizing access to this relation. We could consider * instead postponing application of the restricted quals until we're * 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 - * outer join clauses work correctly. + * that would be a win in very many cases, and it might be tricky to make + * outer join clauses work correctly. It would likely break equivalence + * classes, too. */ if (has_parallel_hazard((Node *) rel->baserestrictinfo, false)) return; @@ -965,9 +980,6 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, continue; } - /* Copy consider_parallel flag from parent. */ - childrel->consider_parallel = rel->consider_parallel; - /* * CE failed, so finish copying/modifying targetlist and join quals. * @@ -1007,6 +1019,16 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, * 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. */ @@ -1023,6 +1045,18 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, /* We have at least one live child. */ 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. */ @@ -1139,6 +1173,15 @@ set_append_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, childRTE = root->simple_rte_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. */ -- 2.40.0