Simplify create_merge_append_path a bit for clarity

Started by Richard Guoover 2 years ago4 messages
#1Richard Guo
guofenglinux@gmail.com
1 attachment(s)

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

#2Alena Rybakina
lena.ribackina@yandex.ru
In reply to: Richard Guo (#1)
Re: Simplify create_merge_append_path a bit for clarity

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

#3Richard Guo
guofenglinux@gmail.com
In reply to: Alena Rybakina (#2)
Re: Simplify create_merge_append_path a bit for clarity

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

#4Richard Guo
guofenglinux@gmail.com
In reply to: Richard Guo (#1)
Re: Simplify create_merge_append_path a bit for clarity

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