From: Tom Lane Date: Wed, 12 Apr 2017 19:13:23 +0000 (-0400) Subject: Mark finished Plan nodes with parallel_safe flags. X-Git-Tag: REL_10_BETA1~291 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=003d80f3dfadd57c6aac8480436ff53ee2c978bd;p=postgresql Mark finished Plan nodes with parallel_safe flags. We'd managed to avoid doing this so far, but it seems pretty obvious that it would be forced on us some day, and this is much the cleanest way of approaching the open problem that parallel-unsafe subplans are being transmitted to parallel workers. Anyway there's no space cost due to alignment considerations, and the time cost is pretty minimal since we're just copying the flag from the corresponding Path node. (At least in most cases ... some of the klugier spots in createplan.c have to work a bit harder.) In principle we could perhaps get rid of SubPlan.parallel_safe, but I thought it better to keep that in case there are reasons to consider a SubPlan unsafe even when its child plan is parallel-safe. This patch doesn't actually do anything with the new flags, but I thought I'd commit it separately anyway. Note: although this touches outfuncs/readfuncs, there's no need for a catversion bump because Plan trees aren't stored on disk. Discussion: https://postgr.es/m/87tw5x4vcu.fsf@credativ.de --- diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 2cb8b3b5ea..00a0fed23d 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -118,6 +118,7 @@ CopyPlanFields(const Plan *from, Plan *newnode) COPY_SCALAR_FIELD(plan_rows); COPY_SCALAR_FIELD(plan_width); COPY_SCALAR_FIELD(parallel_aware); + COPY_SCALAR_FIELD(parallel_safe); COPY_SCALAR_FIELD(plan_node_id); COPY_NODE_FIELD(targetlist); COPY_NODE_FIELD(qual); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 84657cbae9..28cef85579 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -275,6 +275,7 @@ _outPlanInfo(StringInfo str, const Plan *node) WRITE_FLOAT_FIELD(plan_rows, "%.0f"); WRITE_INT_FIELD(plan_width); WRITE_BOOL_FIELD(parallel_aware); + WRITE_BOOL_FIELD(parallel_safe); WRITE_INT_FIELD(plan_node_id); WRITE_NODE_FIELD(targetlist); WRITE_NODE_FIELD(qual); diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index c93a947ee2..a883220a49 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -1480,6 +1480,7 @@ ReadCommonPlan(Plan *local_node) READ_FLOAT_FIELD(plan_rows); READ_INT_FIELD(plan_width); READ_BOOL_FIELD(parallel_aware); + READ_BOOL_FIELD(parallel_safe); READ_INT_FIELD(plan_node_id); READ_NODE_FIELD(targetlist); READ_NODE_FIELD(qual); diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 717b78c161..8179749ef1 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -88,7 +88,7 @@ static Plan *create_unique_plan(PlannerInfo *root, UniquePath *best_path, int flags); static Gather *create_gather_plan(PlannerInfo *root, GatherPath *best_path); static Plan *create_projection_plan(PlannerInfo *root, ProjectionPath *best_path); -static Plan *inject_projection_plan(Plan *subplan, List *tlist); +static Plan *inject_projection_plan(Plan *subplan, List *tlist, bool parallel_safe); static Sort *create_sort_plan(PlannerInfo *root, SortPath *best_path, int flags); static Group *create_group_plan(PlannerInfo *root, GroupPath *best_path); static Unique *create_upper_unique_plan(PlannerInfo *root, UpperUniquePath *best_path, @@ -920,6 +920,9 @@ create_gating_plan(PlannerInfo *root, Path *path, Plan *plan, */ copy_plan_costsize(gplan, plan); + /* Gating quals could be unsafe, so better use the Path's safety flag */ + gplan->parallel_safe = path->parallel_safe; + return gplan; } @@ -1313,7 +1316,8 @@ create_unique_plan(PlannerInfo *root, UniquePath *best_path, int flags) */ if (!is_projection_capable_plan(subplan) && !tlist_same_exprs(newtlist, subplan->targetlist)) - subplan = inject_projection_plan(subplan, newtlist); + subplan = inject_projection_plan(subplan, newtlist, + best_path->path.parallel_safe); else subplan->targetlist = newtlist; } @@ -1572,7 +1576,8 @@ create_projection_plan(PlannerInfo *root, ProjectionPath *best_path) plan->total_cost = best_path->path.total_cost; plan->plan_rows = best_path->path.rows; plan->plan_width = best_path->path.pathtarget->width; - /* ... but be careful not to munge subplan's parallel-aware flag */ + plan->parallel_safe = best_path->path.parallel_safe; + /* ... but don't change subplan's parallel_aware flag */ } else { @@ -1592,9 +1597,12 @@ create_projection_plan(PlannerInfo *root, ProjectionPath *best_path) * This is used in a few places where we decide on-the-fly that we need a * projection step as part of the tree generated for some Path node. * We should try to get rid of this in favor of doing it more honestly. + * + * One reason it's ugly is we have to be told the right parallel_safe marking + * to apply (since the tlist might be unsafe even if the child plan is safe). */ static Plan * -inject_projection_plan(Plan *subplan, List *tlist) +inject_projection_plan(Plan *subplan, List *tlist, bool parallel_safe) { Plan *plan; @@ -1608,6 +1616,7 @@ inject_projection_plan(Plan *subplan, List *tlist) * consistent not more so. Hence, just copy the subplan's cost. */ copy_plan_costsize(plan, subplan); + plan->parallel_safe = parallel_safe; return plan; } @@ -1984,6 +1993,7 @@ create_minmaxagg_plan(PlannerInfo *root, MinMaxAggPath *best_path) plan->plan_rows = 1; plan->plan_width = mminfo->path->pathtarget->width; plan->parallel_aware = false; + plan->parallel_safe = mminfo->path->parallel_safe; /* Convert the plan into an InitPlan in the outer query. */ SS_make_initplan_from_plan(root, subroot, plan, mminfo->param); @@ -2829,6 +2839,7 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual, clamp_row_est(apath->bitmapselectivity * apath->path.parent->tuples); plan->plan_width = 0; /* meaningless */ plan->parallel_aware = false; + plan->parallel_safe = apath->path.parallel_safe; *qual = subquals; *indexqual = subindexquals; *indexECs = subindexECs; @@ -2892,6 +2903,7 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual, clamp_row_est(opath->bitmapselectivity * opath->path.parent->tuples); plan->plan_width = 0; /* meaningless */ plan->parallel_aware = false; + plan->parallel_safe = opath->path.parallel_safe; } /* @@ -2936,6 +2948,7 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual, clamp_row_est(ipath->indexselectivity * ipath->path.parent->tuples); plan->plan_width = 0; /* meaningless */ plan->parallel_aware = false; + plan->parallel_safe = ipath->path.parallel_safe; *qual = get_actual_clauses(ipath->indexclauses); *indexqual = get_actual_clauses(ipath->indexquals); foreach(l, ipath->indexinfo->indpred) @@ -4834,7 +4847,7 @@ order_qual_clauses(PlannerInfo *root, List *clauses) /* * Copy cost and size info from a Path node to the Plan node created from it. * The executor usually won't use this info, but it's needed by EXPLAIN. - * Also copy the parallel-aware flag, which the executor *will* use. + * Also copy the parallel-related flags, which the executor *will* use. */ static void copy_generic_path_info(Plan *dest, Path *src) @@ -4844,6 +4857,7 @@ copy_generic_path_info(Plan *dest, Path *src) dest->plan_rows = src->rows; dest->plan_width = src->pathtarget->width; dest->parallel_aware = src->parallel_aware; + dest->parallel_safe = src->parallel_safe; } /* @@ -4859,6 +4873,8 @@ copy_plan_costsize(Plan *dest, Plan *src) dest->plan_width = src->plan_width; /* Assume the inserted node is not parallel-aware. */ dest->parallel_aware = false; + /* Assume the inserted node is parallel-safe, if child plan is. */ + dest->parallel_safe = src->parallel_safe; } /* @@ -4888,6 +4904,7 @@ label_sort_with_costsize(PlannerInfo *root, Sort *plan, double limit_tuples) plan->plan.plan_rows = lefttree->plan_rows; plan->plan.plan_width = lefttree->plan_width; plan->plan.parallel_aware = false; + plan->plan.parallel_safe = lefttree->parallel_safe; } /* @@ -5696,7 +5713,8 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys, { /* copy needed so we don't modify input's tlist below */ tlist = copyObject(tlist); - lefttree = inject_projection_plan(lefttree, tlist); + lefttree = inject_projection_plan(lefttree, tlist, + lefttree->parallel_safe); } /* Don't bother testing is_projection_capable_plan again */ @@ -5975,6 +5993,7 @@ materialize_finished_plan(Plan *subplan) matplan->plan_rows = subplan->plan_rows; matplan->plan_width = subplan->plan_width; matplan->parallel_aware = false; + matplan->parallel_safe = subplan->parallel_safe; return matplan; } diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 4d5ee01b24..649a233e11 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -351,11 +351,9 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams) /* * Optionally add a Gather node for testing purposes, provided this is - * actually a safe thing to do. (Note: we assume adding a Material node - * above did not change the parallel safety of the plan, so we can still - * rely on best_path->parallel_safe.) + * actually a safe thing to do. */ - if (force_parallel_mode != FORCE_PARALLEL_OFF && best_path->parallel_safe) + if (force_parallel_mode != FORCE_PARALLEL_OFF && top_plan->parallel_safe) { Gather *gather = makeNode(Gather); @@ -378,6 +376,7 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams) gather->plan.plan_rows = top_plan->plan_rows; gather->plan.plan_width = top_plan->plan_width; gather->plan.parallel_aware = false; + gather->plan.parallel_safe = false; /* use parallel mode for parallel plans. */ root->glob->parallelModeNeeded = true; diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 8d0d8aed66..c1be34dd12 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -58,7 +58,7 @@ static Node *build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot, List *plan_params, SubLinkType subLinkType, int subLinkId, Node *testexpr, bool adjust_testexpr, - bool unknownEqFalse, bool parallel_safe); + bool unknownEqFalse); static List *generate_subquery_params(PlannerInfo *root, List *tlist, List **paramIds); static List *generate_subquery_vars(PlannerInfo *root, List *tlist, @@ -550,8 +550,7 @@ make_subplan(PlannerInfo *root, Query *orig_subquery, /* And convert to SubPlan or InitPlan format. */ result = build_subplan(root, plan, subroot, plan_params, subLinkType, subLinkId, - testexpr, true, isTopQual, - best_path->parallel_safe); + testexpr, true, isTopQual); /* * If it's a correlated EXISTS with an unimportant targetlist, we might be @@ -605,8 +604,7 @@ make_subplan(PlannerInfo *root, Query *orig_subquery, plan_params, ANY_SUBLINK, 0, newtestexpr, - false, true, - best_path->parallel_safe)); + false, true)); /* Check we got what we expected */ Assert(hashplan->parParam == NIL); Assert(hashplan->useHashTable); @@ -635,7 +633,7 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot, List *plan_params, SubLinkType subLinkType, int subLinkId, Node *testexpr, bool adjust_testexpr, - bool unknownEqFalse, bool parallel_safe) + bool unknownEqFalse) { Node *result; SubPlan *splan; @@ -654,7 +652,7 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot, &splan->firstColCollation); splan->useHashTable = false; splan->unknownEqFalse = unknownEqFalse; - splan->parallel_safe = parallel_safe; + splan->parallel_safe = plan->parallel_safe; splan->setParam = NIL; splan->parParam = NIL; splan->args = NIL; @@ -1218,7 +1216,8 @@ SS_process_ctes(PlannerInfo *root) /* * CTE scans are not considered for parallelism (cf - * set_rel_consider_parallel). + * set_rel_consider_parallel), and even if they were, initPlans aren't + * parallel-safe. */ splan->parallel_safe = false; splan->setParam = NIL; diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 12f9f615fd..d046e62870 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -124,6 +124,7 @@ typedef struct Plan * information needed for parallel query */ bool parallel_aware; /* engage parallel-aware logic? */ + bool parallel_safe; /* OK to use as part of parallel plan? */ /* * Common structural data for all Plan types.