Fix incorrect comment reference
Hello,
See the attached for a simple comment fix -- the referenced
generate_useful_gather_paths call isn't in grouping_planner it's in
apply_scanjoin_target_to_paths.
Thanks,
James Coleman
Attachments:
v1-0001-Fixup-incorrect-comment.patchapplication/octet-stream; name=v1-0001-Fixup-incorrect-comment.patchDownload
From 162e65db93ca528cd50be9d29640acb9e662871c Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Mon, 23 Jan 2023 08:27:40 -0500
Subject: [PATCH v1] Fixup incorrect comment
---
src/backend/optimizer/path/allpaths.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index c2fc568dc8..ef7c12cfe0 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -3427,7 +3427,8 @@ standard_join_search(PlannerInfo *root, int levels_needed, List *initial_rels)
/*
* Except for the topmost scan/join rel, consider gathering
* partial paths. We'll do the same for the topmost scan/join rel
- * once we know the final targetlist (see grouping_planner).
+ * once we know the final targetlist (see
+ * apply_scanjoin_target_to_paths).
*/
if (!bms_equal(rel->relids, root->all_baserels))
generate_useful_gather_paths(root, rel, false);
--
2.32.1 (Apple Git-133)
On Mon, Jan 23, 2023 at 8:31 AM James Coleman <jtc331@gmail.com> wrote:
See the attached for a simple comment fix -- the referenced
generate_useful_gather_paths call isn't in grouping_planner it's in
apply_scanjoin_target_to_paths.
The intended reading of the comment is not clear. Is it telling you to
look at grouping_planner because that's where we
generate_useful_gather_paths, or is it telling you to look there to
see how we get the final target list together? If it's the former,
then your fix is correct. If the latter, it's fine as it is.
The real answer is probably that some years ago both things happened
in that function. We've moved on from there, but I'm still not sure
what the most useful phrasing of the comment is.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 8:31 AM James Coleman <jtc331@gmail.com> wrote:
See the attached for a simple comment fix -- the referenced
generate_useful_gather_paths call isn't in grouping_planner it's in
apply_scanjoin_target_to_paths.The intended reading of the comment is not clear. Is it telling you to
look at grouping_planner because that's where we
generate_useful_gather_paths, or is it telling you to look there to
see how we get the final target list together? If it's the former,
then your fix is correct. If the latter, it's fine as it is.The real answer is probably that some years ago both things happened
in that function. We've moved on from there, but I'm still not sure
what the most useful phrasing of the comment is.
Yeah, almost certainly, and the comments just didn't keep up.
Would you prefer something that notes both that the broader concern is
happening via the grouping_planner() stage but still points to the
proper callsite (so that people don't go looking for that confused)?
James Coleman
On Mon, Jan 23, 2023 at 3:19 PM James Coleman <jtc331@gmail.com> wrote:
On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 8:31 AM James Coleman <jtc331@gmail.com> wrote:
See the attached for a simple comment fix -- the referenced
generate_useful_gather_paths call isn't in grouping_planner it's in
apply_scanjoin_target_to_paths.The intended reading of the comment is not clear. Is it telling you to
look at grouping_planner because that's where we
generate_useful_gather_paths, or is it telling you to look there to
see how we get the final target list together? If it's the former,
then your fix is correct. If the latter, it's fine as it is.The real answer is probably that some years ago both things happened
in that function. We've moved on from there, but I'm still not sure
what the most useful phrasing of the comment is.Yeah, almost certainly, and the comments just didn't keep up.
Would you prefer something that notes both that the broader concern is
happening via the grouping_planner() stage but still points to the
proper callsite (so that people don't go looking for that confused)?
I don't really have a strong view on what the best thing to do is. I
was just pointing out that the comment might not be quite so obviously
wrong as you were supposing.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Mon, Jan 23, 2023 at 3:41 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 3:19 PM James Coleman <jtc331@gmail.com> wrote:
On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 8:31 AM James Coleman <jtc331@gmail.com> wrote:
See the attached for a simple comment fix -- the referenced
generate_useful_gather_paths call isn't in grouping_planner it's in
apply_scanjoin_target_to_paths.The intended reading of the comment is not clear. Is it telling you to
look at grouping_planner because that's where we
generate_useful_gather_paths, or is it telling you to look there to
see how we get the final target list together? If it's the former,
then your fix is correct. If the latter, it's fine as it is.The real answer is probably that some years ago both things happened
in that function. We've moved on from there, but I'm still not sure
what the most useful phrasing of the comment is.Yeah, almost certainly, and the comments just didn't keep up.
Would you prefer something that notes both that the broader concern is
happening via the grouping_planner() stage but still points to the
proper callsite (so that people don't go looking for that confused)?I don't really have a strong view on what the best thing to do is. I
was just pointing out that the comment might not be quite so obviously
wrong as you were supposing.
"Wrong" is certainly too strong; my apologies.
I'm really just hoping to improve it for future readers to save them
some confusion I had initially reading it.
James Coleman
On Mon, Jan 23, 2023 at 4:07 PM James Coleman <jtc331@gmail.com> wrote:
On Mon, Jan 23, 2023 at 3:41 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 3:19 PM James Coleman <jtc331@gmail.com> wrote:
On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 8:31 AM James Coleman <jtc331@gmail.com> wrote:
See the attached for a simple comment fix -- the referenced
generate_useful_gather_paths call isn't in grouping_planner it's in
apply_scanjoin_target_to_paths.The intended reading of the comment is not clear. Is it telling you to
look at grouping_planner because that's where we
generate_useful_gather_paths, or is it telling you to look there to
see how we get the final target list together? If it's the former,
then your fix is correct. If the latter, it's fine as it is.The real answer is probably that some years ago both things happened
in that function. We've moved on from there, but I'm still not sure
what the most useful phrasing of the comment is.Yeah, almost certainly, and the comments just didn't keep up.
Would you prefer something that notes both that the broader concern is
happening via the grouping_planner() stage but still points to the
proper callsite (so that people don't go looking for that confused)?I don't really have a strong view on what the best thing to do is. I
was just pointing out that the comment might not be quite so obviously
wrong as you were supposing."Wrong" is certainly too strong; my apologies.
I'm really just hoping to improve it for future readers to save them
some confusion I had initially reading it.
Updated patch attached.
Thanks,
James Coleman
Attachments:
v2-0001-Fixup-incorrect-comment.patchapplication/octet-stream; name=v2-0001-Fixup-incorrect-comment.patchDownload
From c2729745c5560fb591d60155868c13b90c0defc7 Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Mon, 23 Jan 2023 08:27:40 -0500
Subject: [PATCH v2] Fixup incorrect comment
---
src/backend/optimizer/path/allpaths.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index c2fc568dc8..689c2e6da5 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -3427,7 +3427,8 @@ standard_join_search(PlannerInfo *root, int levels_needed, List *initial_rels)
/*
* Except for the topmost scan/join rel, consider gathering
* partial paths. We'll do the same for the topmost scan/join rel
- * once we know the final targetlist (see grouping_planner).
+ * once we know the final targetlist (see grouping_planner's and
+ * its call to apply_scanjoin_target_to_paths).
*/
if (!bms_equal(rel->relids, root->all_baserels))
generate_useful_gather_paths(root, rel, false);
--
2.32.1 (Apple Git-133)
On Mon, Jan 23, 2023 at 06:42:45PM -0500, James Coleman wrote:
On Mon, Jan 23, 2023 at 4:07 PM James Coleman <jtc331@gmail.com> wrote:
On Mon, Jan 23, 2023 at 3:41 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 3:19 PM James Coleman <jtc331@gmail.com> wrote:
On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 8:31 AM James Coleman <jtc331@gmail.com> wrote:
See the attached for a simple comment fix -- the referenced
generate_useful_gather_paths call isn't in grouping_planner it's in
apply_scanjoin_target_to_paths.The intended reading of the comment is not clear. Is it telling you to
look at grouping_planner because that's where we
generate_useful_gather_paths, or is it telling you to look there to
see how we get the final target list together? If it's the former,
then your fix is correct. If the latter, it's fine as it is.The real answer is probably that some years ago both things happened
in that function. We've moved on from there, but I'm still not sure
what the most useful phrasing of the comment is.Yeah, almost certainly, and the comments just didn't keep up.
Would you prefer something that notes both that the broader concern is
happening via the grouping_planner() stage but still points to the
proper callsite (so that people don't go looking for that confused)?I don't really have a strong view on what the best thing to do is. I
was just pointing out that the comment might not be quite so obviously
wrong as you were supposing."Wrong" is certainly too strong; my apologies.
I'm really just hoping to improve it for future readers to save them
some confusion I had initially reading it.Updated patch attached.
Patch applied.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
On Fri, Sep 29, 2023 at 2:26 PM Bruce Momjian <bruce@momjian.us> wrote:
On Mon, Jan 23, 2023 at 06:42:45PM -0500, James Coleman wrote:
On Mon, Jan 23, 2023 at 4:07 PM James Coleman <jtc331@gmail.com> wrote:
On Mon, Jan 23, 2023 at 3:41 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 3:19 PM James Coleman <jtc331@gmail.com> wrote:
On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Jan 23, 2023 at 8:31 AM James Coleman <jtc331@gmail.com> wrote:
See the attached for a simple comment fix -- the referenced
generate_useful_gather_paths call isn't in grouping_planner it's in
apply_scanjoin_target_to_paths.The intended reading of the comment is not clear. Is it telling you to
look at grouping_planner because that's where we
generate_useful_gather_paths, or is it telling you to look there to
see how we get the final target list together? If it's the former,
then your fix is correct. If the latter, it's fine as it is.The real answer is probably that some years ago both things happened
in that function. We've moved on from there, but I'm still not sure
what the most useful phrasing of the comment is.Yeah, almost certainly, and the comments just didn't keep up.
Would you prefer something that notes both that the broader concern is
happening via the grouping_planner() stage but still points to the
proper callsite (so that people don't go looking for that confused)?I don't really have a strong view on what the best thing to do is. I
was just pointing out that the comment might not be quite so obviously
wrong as you were supposing."Wrong" is certainly too strong; my apologies.
I'm really just hoping to improve it for future readers to save them
some confusion I had initially reading it.Updated patch attached.
Patch applied.
Thanks!
Regards,
James Coleman