public inbox for [email protected]help / color / mirror / Atom feed
Re: Fix incorrect comment reference 7+ messages / 4 participants [nested] [flat]
* Re: Fix incorrect comment reference @ 2023-01-23 20:19 James Coleman <[email protected]> 2023-01-23 20:41 ` Re: Fix incorrect comment reference Robert Haas <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: James Coleman @ 2023-01-23 20:19 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <[email protected]> wrote: > > On Mon, Jan 23, 2023 at 8:31 AM James Coleman <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix incorrect comment reference 2023-01-23 20:19 Re: Fix incorrect comment reference James Coleman <[email protected]> @ 2023-01-23 20:41 ` Robert Haas <[email protected]> 2023-01-23 21:07 ` Re: Fix incorrect comment reference James Coleman <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Robert Haas @ 2023-01-23 20:41 UTC (permalink / raw) To: James Coleman <[email protected]>; +Cc: pgsql-hackers On Mon, Jan 23, 2023 at 3:19 PM James Coleman <[email protected]> wrote: > On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <[email protected]> wrote: > > On Mon, Jan 23, 2023 at 8:31 AM James Coleman <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix incorrect comment reference 2023-01-23 20:19 Re: Fix incorrect comment reference James Coleman <[email protected]> 2023-01-23 20:41 ` Re: Fix incorrect comment reference Robert Haas <[email protected]> @ 2023-01-23 21:07 ` James Coleman <[email protected]> 2023-01-23 23:42 ` Re: Fix incorrect comment reference James Coleman <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: James Coleman @ 2023-01-23 21:07 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Mon, Jan 23, 2023 at 3:41 PM Robert Haas <[email protected]> wrote: > > On Mon, Jan 23, 2023 at 3:19 PM James Coleman <[email protected]> wrote: > > On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <[email protected]> wrote: > > > On Mon, Jan 23, 2023 at 8:31 AM James Coleman <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix incorrect comment reference 2023-01-23 20:19 Re: Fix incorrect comment reference James Coleman <[email protected]> 2023-01-23 20:41 ` Re: Fix incorrect comment reference Robert Haas <[email protected]> 2023-01-23 21:07 ` Re: Fix incorrect comment reference James Coleman <[email protected]> @ 2023-01-23 23:42 ` James Coleman <[email protected]> 2023-09-29 18:26 ` Re: Fix incorrect comment reference Bruce Momjian <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: James Coleman @ 2023-01-23 23:42 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On Mon, Jan 23, 2023 at 4:07 PM James Coleman <[email protected]> wrote: > > On Mon, Jan 23, 2023 at 3:41 PM Robert Haas <[email protected]> wrote: > > > > On Mon, Jan 23, 2023 at 3:19 PM James Coleman <[email protected]> wrote: > > > On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <[email protected]> wrote: > > > > On Mon, Jan 23, 2023 at 8:31 AM James Coleman <[email protected]> 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: [application/octet-stream] v2-0001-Fixup-incorrect-comment.patch (1.0K, ../../CAAaqYe9F6uoMhAr+8rMLwvGzaKaSknPA0Wi3Ehtv8pbSYmJq-Q@mail.gmail.com/2-v2-0001-Fixup-incorrect-comment.patch) download | inline diff: From c2729745c5560fb591d60155868c13b90c0defc7 Mon Sep 17 00:00:00 2001 From: jcoleman <[email protected]> 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) ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix incorrect comment reference 2023-01-23 20:19 Re: Fix incorrect comment reference James Coleman <[email protected]> 2023-01-23 20:41 ` Re: Fix incorrect comment reference Robert Haas <[email protected]> 2023-01-23 21:07 ` Re: Fix incorrect comment reference James Coleman <[email protected]> 2023-01-23 23:42 ` Re: Fix incorrect comment reference James Coleman <[email protected]> @ 2023-09-29 18:26 ` Bruce Momjian <[email protected]> 2023-09-29 18:38 ` Re: Fix incorrect comment reference James Coleman <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Bruce Momjian @ 2023-09-29 18:26 UTC (permalink / raw) To: James Coleman <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Mon, Jan 23, 2023 at 06:42:45PM -0500, James Coleman wrote: > On Mon, Jan 23, 2023 at 4:07 PM James Coleman <[email protected]> wrote: > > > > On Mon, Jan 23, 2023 at 3:41 PM Robert Haas <[email protected]> wrote: > > > > > > On Mon, Jan 23, 2023 at 3:19 PM James Coleman <[email protected]> wrote: > > > > On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <[email protected]> wrote: > > > > > On Mon, Jan 23, 2023 at 8:31 AM James Coleman <[email protected]> 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 <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Fix incorrect comment reference 2023-01-23 20:19 Re: Fix incorrect comment reference James Coleman <[email protected]> 2023-01-23 20:41 ` Re: Fix incorrect comment reference Robert Haas <[email protected]> 2023-01-23 21:07 ` Re: Fix incorrect comment reference James Coleman <[email protected]> 2023-01-23 23:42 ` Re: Fix incorrect comment reference James Coleman <[email protected]> 2023-09-29 18:26 ` Re: Fix incorrect comment reference Bruce Momjian <[email protected]> @ 2023-09-29 18:38 ` James Coleman <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: James Coleman @ 2023-09-29 18:38 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Fri, Sep 29, 2023 at 2:26 PM Bruce Momjian <[email protected]> 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 <[email protected]> wrote: > > > > > > On Mon, Jan 23, 2023 at 3:41 PM Robert Haas <[email protected]> wrote: > > > > > > > > On Mon, Jan 23, 2023 at 3:19 PM James Coleman <[email protected]> wrote: > > > > > On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <[email protected]> wrote: > > > > > > On Mon, Jan 23, 2023 at 8:31 AM James Coleman <[email protected]> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v27 8/9] Row pattern recognition patch (typedefs.list). @ 2024-12-30 23:53 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Tatsuo Ishii @ 2024-12-30 23:53 UTC (permalink / raw) --- src/tools/pgindent/typedefs.list | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index e1c4f913f8..72906d0fc1 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -1691,6 +1691,7 @@ NamedLWLockTrancheRequest NamedTuplestoreScan NamedTuplestoreScanState NamespaceInfo +NavigationInfo NestLoop NestLoopParam NestLoopState @@ -2317,6 +2318,9 @@ RI_CompareKey RI_ConstraintInfo RI_QueryHashEntry RI_QueryKey +RPCommonSyntax +RPSkipTo +RPSubsetItem RTEKind RTEPermissionInfo RWConflict @@ -2673,6 +2677,7 @@ SimpleStringList SimpleStringListCell SingleBoundSortItem Size +SkipContext SkipPages SlabBlock SlabContext @@ -2764,6 +2769,7 @@ StreamStopReason String StringInfo StringInfoData +StringSet StripnullState SubLink SubLinkType @@ -3088,6 +3094,7 @@ VarString VarStringSortSupport Variable VariableAssignHook +VariablePos VariableSetKind VariableSetStmt VariableShowStmt -- 2.25.1 ----Next_Part(Tue_Dec_31_08_57_07_2024_963)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v27-0009-Allow-to-print-raw-parse-tree.patch" ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-12-30 23:53 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-01-23 20:19 Re: Fix incorrect comment reference James Coleman <[email protected]> 2023-01-23 20:41 ` Robert Haas <[email protected]> 2023-01-23 21:07 ` James Coleman <[email protected]> 2023-01-23 23:42 ` James Coleman <[email protected]> 2023-09-29 18:26 ` Bruce Momjian <[email protected]> 2023-09-29 18:38 ` James Coleman <[email protected]> 2024-12-30 23:53 [PATCH v27 8/9] Row pattern recognition patch (typedefs.list). Tatsuo Ishii <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox