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

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 535d2e69b2b166355cba4f32f906a223cd927cf7..69c7cf64d55d0f375ce34c93a6242395b3e0184a 100644 (file)
--- 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.
                 */