public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Tom Lane <[email protected]>
Cc: Yura Sokolov <[email protected]>
Cc: PostgreSQL Developers <[email protected]>
Subject: Re: Fix BUG #17335: Duplicate result rows in Gather node
Date: Fri, 4 Feb 2022 13:07:42 +1300
Message-ID: <CAApHDvqAu-20gMaiHfHyOKdZ1279bcFvWPoE9a+RF3hWSYeifQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CAApHDvqMUshyci-F2CZ-jkSbmuhzCudhF5t5_z0shFxd9D_hVg@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAApHDvrxnqQdqXQ1BxTRP5V+8pxBF+GMMhuJ0Qq8J2bT=Uwi+A@mail.gmail.com>
<CAApHDvqEnKn_Y4nNPqCPRLkwxZWS3tNPU6VxB9P1QtiN8y3onw@mail.gmail.com>
<[email protected]>
<[email protected]>
On Wed, 26 Jan 2022 at 05:32, Tom Lane <[email protected]> wrote:
> Therefore, what I think could be useful is some very-late-stage
> assertion check (probably in createplan.c) verifying that the
> child of a Gather is parallel-aware. Or maybe the condition
> needs to be more general than that, but anyway the idea is for
> the back end of the planner to verify that we didn't build a
> silly plan.
I had a go at writing something along these lines, but I've ended up
with something I really don't like very much.
I ended up having to write a recursive path traversal function. It's
generic and it can be given a callback function to do whatever we like
with the Path. The problem is, that this seems like quite a bit of
code to maintain just for plan validation in Assert builds.
Currently, the patch validates 3 rules:
1) Ensure a parallel_aware path has only parallel_aware or
parallel_safe subpaths.
2) Ensure Gather is either single_copy or contains at least one
parallel_aware subnode.
3) Ensure GatherMerge contains at least one parallel_aware subnode.
I had to relax rule #1 a little as a Parallel Append can run subnodes
that are only parallel_safe and not parallel_aware. The problem with
relaxing this rule is that it does not catch the case that this bug
report was about. I could maybe tweak that so there's a special case
for Append to allow parallel aware or safe and ensure all other nodes
have only parallel_safe subnodes. I just don't really like that
special case as it's likely to get broken/forgotten over time when we
add new nodes.
I'm unsure if just being able to enforce rules #2 and #3 make this worthwhile.
Happy to listen to other people's opinions and ideas on this. Without
those, I'm unlikely to try to push this any further.
David
Attachments:
[application/octet-stream] do_some_plan_validation_during_create_plan.patch (7.6K, ../CAApHDvqAu-20gMaiHfHyOKdZ1279bcFvWPoE9a+RF3hWSYeifQ@mail.gmail.com/2-do_some_plan_validation_during_create_plan.patch)
download | inline diff:
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index cd6d72c763..56e28ba739 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -313,6 +313,21 @@ static ModifyTable *make_modifytable(PlannerInfo *root, Plan *subplan,
List *rowMarks, OnConflictExpr *onconflict, int epqParam);
static GatherMerge *create_gather_merge_plan(PlannerInfo *root,
GatherMergePath *best_path);
+static bool contains_a_parallel_path(Path *path);
+static bool contains_only_parallel_aware_and_safe_paths(Path *path);
+
+/*
+ * PathTypeCount
+ * Used for various checks to assert plans are sane in assert enabled
+ * builds.
+ */
+typedef struct PathTypeCount
+{
+ uint64 count;
+ uint64 parallel_safe_count;
+ uint64 parallel_aware_count;
+
+} PathTypeCount;
/*
@@ -389,6 +404,10 @@ create_plan_recurse(PlannerInfo *root, Path *best_path, int flags)
/* Guard against stack overflow due to overly complex plans */
check_stack_depth();
+ /* Parallel aware paths should contain only parallel aware subpaths. */
+ Assert(!best_path->parallel_aware ||
+ contains_only_parallel_aware_and_safe_paths(best_path));
+
switch (best_path->pathtype)
{
case T_SeqScan:
@@ -481,6 +500,13 @@ create_plan_recurse(PlannerInfo *root, Path *best_path, int flags)
case T_Gather:
plan = (Plan *) create_gather_plan(root,
(GatherPath *) best_path);
+
+ /*
+ * We expect a Gather to contain at least one parallel path unless
+ * running in single_copy mode.
+ */
+ Assert(((GatherPath *) best_path)->single_copy ||
+ contains_a_parallel_path(((GatherPath *) best_path)->subpath));
break;
case T_Sort:
plan = (Plan *) create_sort_plan(root,
@@ -537,6 +563,8 @@ create_plan_recurse(PlannerInfo *root, Path *best_path, int flags)
case T_GatherMerge:
plan = (Plan *) create_gather_merge_plan(root,
(GatherMergePath *) best_path);
+ /* We expect a GatherMerge to contain at least one parallel path */
+ Assert(contains_a_parallel_path(((GatherMergePath *) best_path)->subpath));
break;
default:
elog(ERROR, "unrecognized node type: %d",
@@ -7052,6 +7080,210 @@ make_modifytable(PlannerInfo *root, Plan *subplan,
return node;
}
+/*
+ * path_tree_walker
+ * Walk a path tree beginning with 'path' and call the 'walker' function
+ * for that path and each of its subpaths recursively.
+ */
+static void
+path_tree_walker(Path *path, void (*walker) (), void *context)
+
+{
+ if (path == NULL)
+ return;
+
+ /* Guard against stack overflow due to overly complex path trees */
+ check_stack_depth();
+
+ walker(path, context);
+
+ switch (path->pathtype)
+ {
+ case T_SeqScan:
+ case T_SampleScan:
+ case T_IndexScan:
+ case T_IndexOnlyScan:
+ case T_BitmapHeapScan:
+ case T_TidScan:
+ case T_TidRangeScan:
+ case T_SubqueryScan:
+ case T_FunctionScan:
+ case T_TableFuncScan:
+ case T_ValuesScan:
+ case T_CteScan:
+ case T_WorkTableScan:
+ case T_NamedTuplestoreScan:
+ case T_ForeignScan:
+ case T_CustomScan:
+ /* Scan paths have no subpaths */
+ break;
+ case T_HashJoin:
+ case T_MergeJoin:
+ case T_NestLoop:
+ path_tree_walker(((JoinPath *) path)->outerjoinpath, walker, context);
+ path_tree_walker(((JoinPath *) path)->innerjoinpath, walker, context);
+ break;
+ case T_Append:
+ {
+ AppendPath *apath = (AppendPath *) path;
+ ListCell *lc;
+
+ foreach(lc, apath->subpaths)
+ {
+ Path *subpath = lfirst(lc);
+
+ path_tree_walker(subpath, walker, context);
+ }
+ }
+ break;
+ case T_MergeAppend:
+ {
+ MergeAppendPath *mpath = (MergeAppendPath *) path;
+ ListCell *lc;
+
+ foreach(lc, mpath->subpaths)
+ {
+ Path *subpath = lfirst(lc);
+
+ path_tree_walker(subpath, walker, context);
+ }
+ }
+ break;
+ case T_Result:
+ if (IsA(path, ProjectionPath))
+ {
+ path_tree_walker(((ProjectionPath *) path)->subpath, walker, context);
+ }
+ else if (IsA(path, MinMaxAggPath))
+ {
+ /* MinMaxAggPath has no subpaths */
+ }
+ else if (IsA(path, GroupResultPath))
+ {
+ /* GroupResultPath has no subpaths */
+ }
+ else
+ {
+ /* No subpaths for any other Result type path */
+ }
+ break;
+ case T_ProjectSet:
+ path_tree_walker(((ProjectSetPath *) path)->subpath, walker, context);
+ break;
+ case T_Material:
+ path_tree_walker(((MaterialPath *) path)->subpath, walker, context);
+ break;
+ case T_Memoize:
+ path_tree_walker(((MemoizePath *) path)->subpath, walker, context);
+ break;
+ case T_Unique:
+ if (IsA(path, UpperUniquePath))
+ {
+ path_tree_walker(((UpperUniquePath *) path)->subpath, walker, context);
+ }
+ else
+ {
+ Assert(IsA(path, UniquePath));
+ path_tree_walker(((UniquePath *) path)->subpath, walker, context);
+ }
+ break;
+ case T_Gather:
+ path_tree_walker(((GatherPath *) path)->subpath, walker, context);
+ break;
+ case T_Sort:
+ path_tree_walker(((SortPath *) path)->subpath, walker, context);
+ break;
+ case T_IncrementalSort:
+ path_tree_walker(((IncrementalSortPath *) path)->spath.subpath, walker, context);
+ break;
+ case T_Group:
+ path_tree_walker(((GroupPath *) path)->subpath, walker, context);
+ break;
+ case T_Agg:
+ if (IsA(path, GroupingSetsPath))
+ path_tree_walker(((GroupingSetsPath *) path)->subpath, walker, context);
+ else
+ {
+ Assert(IsA(path, AggPath));
+ path_tree_walker(((AggPath *) path)->subpath, walker, context);
+ }
+ break;
+ case T_WindowAgg:
+ path_tree_walker(((WindowAggPath *) path)->subpath, walker, context);
+ break;
+ case T_SetOp:
+ path_tree_walker(((SetOpPath *) path)->subpath, walker, context);
+ break;
+ case T_RecursiveUnion:
+ path_tree_walker(((RecursiveUnionPath *) path)->leftpath, walker, context);
+ path_tree_walker(((RecursiveUnionPath *) path)->rightpath, walker, context);
+ break;
+ case T_LockRows:
+ path_tree_walker(((LockRowsPath *) path)->subpath, walker, context);
+ break;
+ case T_ModifyTable:
+ path_tree_walker(((ModifyTablePath *) path)->subpath, walker, context);
+ break;
+ case T_Limit:
+ path_tree_walker(((LimitPath *) path)->subpath, walker, context);
+ break;
+ case T_GatherMerge:
+ path_tree_walker(((GatherMergePath *) path)->subpath, walker, context);
+ break;
+ default:
+ elog(ERROR, "unrecognized node type: %d", (int) path->pathtype);
+ break;
+ }
+}
+
+/*
+ * path_type_counter
+ * Determine the total number of paths and the number of paths that are
+ * parallel_aware and the number that are parallel safe.
+ */
+static void
+path_type_counter(Path *path, PathTypeCount *pathcount)
+{
+ pathcount->count++;
+ if (path->parallel_aware)
+ pathcount->parallel_aware_count++;
+ else if (path->parallel_safe)
+ pathcount->parallel_safe_count++;
+}
+
+/*
+ * contains_a_parallel_path
+ * Determine if 'path' or any of its subpaths are parallel aware
+ */
+static bool
+contains_a_parallel_path(Path *path)
+{
+ PathTypeCount pathcount;
+
+ memset(&pathcount, 0, sizeof(pathcount));
+
+ path_tree_walker(path, path_type_counter, (void *) &pathcount);
+
+ return (pathcount.parallel_aware_count > 0);
+}
+
+/*
+ * contains_only_parallel_aware_and_safe_paths
+ * Returns true if 'path' and all of its subpaths are either parallel
+ * aware or parallel safe.
+ */
+static bool
+contains_only_parallel_aware_and_safe_paths(Path *path)
+{
+ PathTypeCount pathcount;
+
+ memset(&pathcount, 0, sizeof(pathcount));
+
+ path_tree_walker(path, path_type_counter, (void *) &pathcount);
+
+ return (pathcount.parallel_aware_count + pathcount.parallel_safe_count == pathcount.count);
+}
+
/*
* is_projection_capable_path
* Check whether a given Path node is able to do projection.
view thread (15+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Fix BUG #17335: Duplicate result rows in Gather node
In-Reply-To: <CAApHDvqAu-20gMaiHfHyOKdZ1279bcFvWPoE9a+RF3hWSYeifQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox