From 003d80f3dfadd57c6aac8480436ff53ee2c978bd Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 12 Apr 2017 15:13:23 -0400
Subject: [PATCH] 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
---
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/outfuncs.c | 1 +
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/plan/createplan.c | 31 ++++++++++++++++++++-----
src/backend/optimizer/plan/planner.c | 7 +++---
src/backend/optimizer/plan/subselect.c | 15 ++++++------
src/include/nodes/plannodes.h | 1 +
7 files changed, 39 insertions(+), 18 deletions(-)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 2cb8b3b5ea2..00a0fed23d4 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 84657cbae92..28cef85579c 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 c93a947ee26..a883220a490 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 717b78c161a..8179749ef12 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 4d5ee01b248..649a233e119 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 8d0d8aed663..c1be34dd12c 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 12f9f615fde..d046e628702 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.
--
GitLab