public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Yura Sokolov <[email protected]>
Cc: PostgreSQL Developers <[email protected]>
Subject: Re: Fix BUG #17335: Duplicate result rows in Gather node
Date: Thu, 20 Jan 2022 09:32:17 +1300
Message-ID: <CAApHDvqMUshyci-F2CZ-jkSbmuhzCudhF5t5_z0shFxd9D_hVg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
On Fri, 31 Dec 2021 at 00:14, Yura Sokolov <[email protected]> wrote:
> Suggested quick (and valid) fix in the patch attached:
> - If Append has single child, then copy its parallel awareness.
I've been looking at this and I've gone through changing my mind about
what's the right fix quite a number of times.
My current thoughts are that I don't really like the fact that we can
have plans in the following shape:
Finalize Aggregate
-> Gather
Workers Planned: 1
-> Partial Aggregate
-> Parallel Hash Left Join
Hash Cond: (gather_append_1.fk = gather_append_2.fk)
-> Index Scan using gather_append_1_ix on gather_append_1
Index Cond: (f = true)
-> Parallel Hash
-> Parallel Seq Scan on gather_append_2
It's only made safe by the fact that Gather will only use 1 worker.
To me, it just seems too fragile to assume that's always going to be
the case. I feel like this fix just relies on the fact that
create_gather_path() and create_gather_merge_path() do
"pathnode->num_workers = subpath->parallel_workers;". If someone
decided that was to work a different way, then we risk this breaking
again. Additionally, today we have Gather and GatherMerge, but we may
one day end up with more node types that gather results from parallel
workers, or even a completely different way of executing plans.
I think a safer way to fix this is to just not remove the
Append/MergeAppend node if the parallel_aware flag of the only-child
and the Append/MergeAppend don't match. I've done that in the
attached.
I believe the code at the end of add_paths_to_append_rel() can remain as is.
David
Attachments:
[application/octet-stream] dont_remove_singlechild_appends_with_mismatching_parallel_awareness.patch (2.0K, ../CAApHDvqMUshyci-F2CZ-jkSbmuhzCudhF5t5_z0shFxd9D_hVg@mail.gmail.com/2-dont_remove_singlechild_appends_with_mismatching_parallel_awareness.patch)
download | inline diff:
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index e44ae971b4..a7b11b7f03 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1512,8 +1512,16 @@ set_append_references(PlannerInfo *root,
lfirst(l) = set_plan_refs(root, (Plan *) lfirst(l), rtoffset);
}
- /* Now, if there's just one, forget the Append and return that child */
- if (list_length(aplan->appendplans) == 1)
+ /*
+ * See if it's safe to get rid of the Append entirely. For this to be
+ * safe, there must be only one child plan and that child plan's parallel
+ * awareness must match that of the Append's. The reason for the latter
+ * is that the if the Append is parallel aware and the child is not then
+ * the calling plan may execute the non-parallel aware child multiple
+ * times.
+ */
+ if (list_length(aplan->appendplans) == 1 &&
+ ((Plan *) linitial(aplan->appendplans))->parallel_aware == aplan->plan.parallel_aware)
return clean_up_removed_plan_level((Plan *) aplan,
(Plan *) linitial(aplan->appendplans));
@@ -1576,8 +1584,16 @@ set_mergeappend_references(PlannerInfo *root,
lfirst(l) = set_plan_refs(root, (Plan *) lfirst(l), rtoffset);
}
- /* Now, if there's just one, forget the MergeAppend and return that child */
- if (list_length(mplan->mergeplans) == 1)
+ /*
+ * See if it's safe to get rid of the MergeAppend entirely. For this to
+ * be safe, there must be only one child plan and that child plan's
+ * parallel awareness must match that of the MergeAppend's. The reason
+ * for the latter is that the if the MergeAppend is parallel aware and the
+ * child is not then the calling plan may execute the non-parallel aware
+ * child multiple times.
+ */
+ if (list_length(mplan->mergeplans) == 1 &&
+ ((Plan *) linitial(mplan->mergeplans))->parallel_aware == mplan->plan.parallel_aware)
return clean_up_removed_plan_level((Plan *) mplan,
(Plan *) linitial(mplan->mergeplans));
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]
Subject: Re: Fix BUG #17335: Duplicate result rows in Gather node
In-Reply-To: <CAApHDvqMUshyci-F2CZ-jkSbmuhzCudhF5t5_z0shFxd9D_hVg@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