public inbox for [email protected]
help / color / mirror / Atom feedFrom: Richard Guo <[email protected]>
To: PostgreSQL-development <[email protected]>
Subject: Re: Redundant code in create_gather_merge_path
Date: Thu, 18 Jul 2024 11:08:48 +0800
Message-ID: <CAMbWs4_V0Zr0ckUgC8Sz_GezjVPVQA31EjLHiaSgT1P3h2unsw@mail.gmail.com> (raw)
In-Reply-To: <CAMbWs48u=0bWf3epVtULjJ-=M9Hbkz+ieZQAOS=BfbXZFqbDCg@mail.gmail.com>
References: <CAMbWs48u=0bWf3epVtULjJ-=M9Hbkz+ieZQAOS=BfbXZFqbDCg@mail.gmail.com>
On Thu, Jul 18, 2024 at 10:02 AM Richard Guo <[email protected]> wrote:
> I noticed this while reviewing patch [1], thinking that it might be
> worth fixing. Any thoughts?
Here is the patch.
Thanks
Richard
Attachments:
[application/octet-stream] v1-0001-Remove-redundant-code-in-create_gather_merge_path.patch (4.1K, ../CAMbWs4_V0Zr0ckUgC8Sz_GezjVPVQA31EjLHiaSgT1P3h2unsw@mail.gmail.com/2-v1-0001-Remove-redundant-code-in-create_gather_merge_path.patch)
download | inline diff:
From df8ffdd28e7b65b5e11d8f531722983bfbf8eb2c Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Thu, 18 Jul 2024 11:52:48 +0900
Subject: [PATCH v1] Remove redundant code in create_gather_merge_path
In create_gather_merge_path, we should always guarantee that the
subpath is adequately ordered, and we do not add a Sort node in
createplan.c for a Gather Merge node. Therefore, the 'else' branch in
create_gather_merge_path, which computes the cost for a Sort node, is
redundant.
This patch removes the redundant code and emits an error if the
subpath is not sufficiently ordered. Meanwhile, this patch changes
the check for the subpath's pathkeys in create_gather_merge_plan to an
Assert.
Author: Richard Guo
Discussion: https://postgr.es/m/CAMbWs48u=0bWf3epVtULjJ-=M9Hbkz+ieZQAOS=BfbXZFqbDCg@mail.gmail.com
---
src/backend/optimizer/plan/createplan.c | 9 ++-----
src/backend/optimizer/util/pathnode.c | 35 +++++++++----------------
2 files changed, 14 insertions(+), 30 deletions(-)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 6b64c4a362..fe5a323cfd 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1987,16 +1987,11 @@ create_gather_merge_plan(PlannerInfo *root, GatherMergePath *best_path)
&gm_plan->collations,
&gm_plan->nullsFirst);
-
/*
* All gather merge paths should have already guaranteed the necessary
- * sort order either by adding an explicit sort node or by using presorted
- * input. We can't simply add a sort here on additional pathkeys, because
- * we can't guarantee the sort would be safe. For example, expressions may
- * be volatile or otherwise parallel unsafe.
+ * sort order. See create_gather_merge_path.
*/
- if (!pathkeys_contained_in(pathkeys, best_path->subpath->pathkeys))
- elog(ERROR, "gather merge input not sufficiently sorted");
+ Assert(pathkeys_contained_in(pathkeys, best_path->subpath->pathkeys));
/* Now insert the subplan under GatherMerge. */
gm_plan->plan.lefttree = subplan;
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index c42742d2c7..6182e43b37 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1889,6 +1889,16 @@ create_gather_merge_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath,
Assert(subpath->parallel_safe);
Assert(pathkeys);
+ /*
+ * The subpath should guarantee that it is adequately ordered either by
+ * adding an explicit sort node or by using presorted input. We cannot add
+ * an explicit Sort node for the subpath in createplan.c on additional
+ * pathkeys, because we can't guarantee the sort would be safe. For
+ * example, expressions may be volatile or otherwise parallel unsafe.
+ */
+ if (!pathkeys_contained_in(pathkeys, subpath->pathkeys))
+ elog(ERROR, "gather merge input not sufficiently sorted");
+
pathnode->path.pathtype = T_GatherMerge;
pathnode->path.parent = rel;
pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
@@ -1901,29 +1911,8 @@ create_gather_merge_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath,
pathnode->path.pathtarget = target ? target : rel->reltarget;
pathnode->path.rows += subpath->rows;
- if (pathkeys_contained_in(pathkeys, subpath->pathkeys))
- {
- /* Subpath is adequately ordered, we won't need to sort it */
- input_startup_cost += subpath->startup_cost;
- input_total_cost += subpath->total_cost;
- }
- else
- {
- /* We'll need to insert a Sort node, so include cost for that */
- Path sort_path; /* dummy for result of cost_sort */
-
- cost_sort(&sort_path,
- root,
- pathkeys,
- subpath->total_cost,
- subpath->rows,
- subpath->pathtarget->width,
- 0.0,
- work_mem,
- -1);
- input_startup_cost += sort_path.startup_cost;
- input_total_cost += sort_path.total_cost;
- }
+ input_startup_cost += subpath->startup_cost;
+ input_total_cost += subpath->total_cost;
cost_gather_merge(pathnode, root, rel, pathnode->path.param_info,
input_startup_cost, input_total_cost, rows);
--
2.43.0
view thread (3+ messages)
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]
Subject: Re: Redundant code in create_gather_merge_path
In-Reply-To: <CAMbWs4_V0Zr0ckUgC8Sz_GezjVPVQA31EjLHiaSgT1P3h2unsw@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