Simplify create_merge_append_path a bit for clarity
As explained in the comments for generate_orderedappend_paths, we don't
currently support parameterized MergeAppend paths, and it doesn't seem
like going to change anytime soon. Based on that, we could simplify
create_merge_append_path a bit, such as set param_info to NULL directly
rather than call get_appendrel_parampathinfo() for it. We already have
an Assert on that in create_merge_append_plan.
I understand that the change would not make any difference for
performance, it's just for clarity's sake.
Thanks
Richard
Attachments:
v1-0001-Simplify-create_merge_append_path-a-bit-for-clarity.patchapplication/octet-stream; name=v1-0001-Simplify-create_merge_append_path-a-bit-for-clarity.patchDownload
From ac02c2557f654b9ceaf582f5c79878bec80953a0 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 11 Aug 2023 09:08:20 +0800
Subject: [PATCH v1] Simplify create_merge_append_path a bit for clarity
---
src/backend/optimizer/util/pathnode.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 754f0b9f34..dd259322e0 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -1421,11 +1421,16 @@ create_merge_append_path(PlannerInfo *root,
Cost input_total_cost;
ListCell *l;
+ /*
+ * We don't currently support parameterized MergeAppend paths, as explained
+ * in the comments for generate_orderedappend_paths.
+ */
+ Assert(bms_is_empty(rel->lateral_relids) && bms_is_empty(required_outer));
+
pathnode->path.pathtype = T_MergeAppend;
pathnode->path.parent = rel;
pathnode->path.pathtarget = rel->reltarget;
- pathnode->path.param_info = get_appendrel_parampathinfo(rel,
- required_outer);
+ pathnode->path.param_info = NULL;
pathnode->path.parallel_aware = false;
pathnode->path.parallel_safe = rel->consider_parallel;
pathnode->path.parallel_workers = 0;
@@ -1451,6 +1456,9 @@ create_merge_append_path(PlannerInfo *root,
{
Path *subpath = (Path *) lfirst(l);
+ /* All child paths should be unparameterized */
+ Assert(bms_is_empty(PATH_REQ_OUTER(subpath)));
+
pathnode->path.rows += subpath->rows;
pathnode->path.parallel_safe = pathnode->path.parallel_safe &&
subpath->parallel_safe;
@@ -1478,9 +1486,6 @@ create_merge_append_path(PlannerInfo *root,
input_startup_cost += sort_path.startup_cost;
input_total_cost += sort_path.total_cost;
}
-
- /* All child paths must have same parameterization */
- Assert(bms_equal(PATH_REQ_OUTER(subpath), required_outer));
}
/*
--
2.31.0
Hi!
On 11.08.2023 05:31, Richard Guo wrote:
As explained in the comments for generate_orderedappend_paths, we don't
currently support parameterized MergeAppend paths, and it doesn't seem
like going to change anytime soon. Based on that, we could simplify
create_merge_append_path a bit, such as set param_info to NULL directly
rather than call get_appendrel_parampathinfo() for it. We already have
an Assert on that in create_merge_append_plan.I understand that the change would not make any difference for
performance, it's just for clarity's sake.
I agree with you, and we can indeed directly set the param_info value to
NULL, and there are enough comments here to explain.
I didn't find anything else to add in your patch.
--
Regards,
Alena Rybakina
On Tue, Oct 24, 2023 at 6:00 PM Alena Rybakina <lena.ribackina@yandex.ru>
wrote:
I agree with you, and we can indeed directly set the param_info value to
NULL, and there are enough comments here to explain.I didn't find anything else to add in your patch.
Thanks for reviewing this patch!
Thanks
Richard
On Fri, Jul 26, 2024 at 1:28 PM Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:
Is there a reason you don't want to remove the required_outer
parameter altogether? I guess because it is such a common pattern to
pass it?
I think it's best to keep this parameter unchanged to maintain
consistency with other functions that create path nodes in pathnode.c.
Do you think it is worth keeping this assertion?:
-
- /* All child paths must have same parameterization */
- Assert(bms_equal(PATH_REQ_OUTER(subpath), required_outer));I understand any failure would trigger one of the prior asserts
instead, but it does communicate an extra requirement, and there is no
cost.
I don't think it's a good idea to keep this Assert: with this change
it becomes redundant.
But I'd be fine with committing this patch as-is.
I've pushed this patch. Thanks for review.
Thanks
Richard
Import Notes
Reply to msg id not found: CA+renyVUKH0FrHaujdg4p46yjy_1+q+BRXjZeiPYqcNSo07mA@mail.gmail.com