Re: Improve join_search_one_level readibilty (one line change)
On Fri, 4 Aug 2023 at 16:05, Richard Guo wrote: > > > On Fri, Aug 4, 2023 at 10:36 AM David Rowley wrote: >> >> The whole lnext() stuff all feels a bit old now that Lists are arrays. >> I think we'd be better adjusting the code to pass the List index where >> we start from rather than the ListCell to start from. That way we can >> use for_each_from() to iterate rather than for_each_cell(). What's >> there today feels a bit crufty and there's some element of danger that >> the given ListCell does not even belong to the given List. > > > I think we can go even further to do the same for 'bushy plans' case, > like the attached. Seems like a good idea to me. I've pushed that patch. Alex, many thanks for highlighting this and posting a patch to fix it. Congratulations on your first patch being committed. David
Re: Improve join_search_one_level readibilty (one line change)
On Fri, Aug 4, 2023 at 10:36 AM David Rowley wrote: > The whole lnext() stuff all feels a bit old now that Lists are arrays. > I think we'd be better adjusting the code to pass the List index where > we start from rather than the ListCell to start from. That way we can > use for_each_from() to iterate rather than for_each_cell(). What's > there today feels a bit crufty and there's some element of danger that > the given ListCell does not even belong to the given List. I think we can go even further to do the same for 'bushy plans' case, like the attached. Thanks Richard v2-0001-join_search_one_level-tidyup.patch Description: Binary data
Re: Improve join_search_one_level readibilty (one line change)
On Tue, 1 Aug 2023 at 01:48, Julien Rouhaud wrote: > Apart from that +1 from me for the patch, I think it helps focusing the > attention on what actually matters here. I think it's worth doing something to improve this code. However, I think we should go a bit further than what the proposed patch does. In 1cff1b95a, Tom changed the signature of make_rels_by_clause_joins to pass the List that the given ListCell belongs to. This was done because lnext(), as of that commit, requires the owning List to be passed to the function, where previously when Lists were linked lists, that wasn't required. The whole lnext() stuff all feels a bit old now that Lists are arrays. I think we'd be better adjusting the code to pass the List index where we start from rather than the ListCell to start from. That way we can use for_each_from() to iterate rather than for_each_cell(). What's there today feels a bit crufty and there's some element of danger that the given ListCell does not even belong to the given List. Doing this seems to shrink down the assembly a bit: $ wc -l joinrels* 3344 joinrels_tidyup.s 3363 joinrels_unpatched.s I also see a cmovne in joinrels_tidyup.s, which means that there are fewer jumps which makes things a little better for the branch predictor as there's fewer jumps. I doubt this is going to be performance critical, but it's a nice extra bonus to go along with the cleanup. old: cmpl $2, 24(%rsp) je .L616 new: cmpl $2, 16(%rsp) cmovne %edx, %eax David join_search_one_level_tidyup.patch Description: Binary data
Re: Improve join_search_one_level readibilty (one line change)
Hi,
On Wed, Jun 07, 2023 at 11:02:09AM +0800, 謝東霖 wrote:
> Thank you, Julien, for letting me know that cfbot doesn't test txt files.
> Much appreciated!
Thanks for posting this v2!
So unsurprisingly the cfbot is happy with this patch, since it doesn't change
the behavior at all. I just have some nitpicking:
@@ -109,14 +109,14 @@ join_search_one_level(PlannerInfo *root, int level)
List *other_rels_list;
ListCell *other_rels;
+ other_rels_list = joinrels[1];
+
if (level == 2) /* consider remaining initial
rels */
{
- other_rels_list = joinrels[level - 1];
other_rels = lnext(other_rels_list, r);
}
else/* consider all initial
rels */
{
- other_rels_list = joinrels[1];
other_rels = list_head(other_rels_list);
}
Since each branch only has a single instruction after the change the curly
braces aren't needed anymore. The only reason keep them is if it helps
readability (like if there's a big comment associated), but that's not the case
here so it would be better to get rid of them.
Apart from that +1 from me for the patch, I think it helps focusing the
attention on what actually matters here.
Re: Improve join_search_one_level readibilty (one line change)
On Wed Jun 7, 2023 at 10:05 AM CDT, 謝東霖 wrote: > Peter Eisentraut > > That shows exactly the problem being complained about. > > I apologize for not using the correct MIME type in my previous email > to the pg-hackers mailing list. Upon sending the first email, I > realized that my patch was labeled as "application/x-patch" instead of > "text/x-patch." > > To make it more convenient for others to read the patch in the mail > archives, I changed the file extension of my v2 patch to ".txt." ( > https://www.postgresql.org/message-id/CANWNU8xm07jYUHxGh3XNHtcY37z%2B56-6bDD4piPt6%3DKidiHshQ%40mail.gmail.com > ) > > However, I encountered an issue where cfbot did not apply the ".txt" > file. I am still interested in learning **how to submit patches with > the proper "text/x-patch" MIME type using Gmail.** > > I have noticed that some individuals have successfully used Gmail to > submit patches with the correct MIME type. > > If anyone can provide assistance with this matter, I would be > grateful. I am willing to contribute any helpful tips regarding using > Gmail for patch submission to the Postgres wiki. > > I believe it is something like Mutt, right? Might be a good idea to reach out to the people directly about how they do it. I am using Gmail with this work account, but I am using aerc to interact with the list. I could rant all day about Gmail :). All of my patches seem to get the right MIME types. Could be worth looking into if you like terminal-based workflows at all. -- Tristan Partin Neon (https://neon.tech)
Re: Improve join_search_one_level readibilty (one line change)
Peter Eisentraut > That shows exactly the problem being complained about. I apologize for not using the correct MIME type in my previous email to the pg-hackers mailing list. Upon sending the first email, I realized that my patch was labeled as "application/x-patch" instead of "text/x-patch." To make it more convenient for others to read the patch in the mail archives, I changed the file extension of my v2 patch to ".txt." ( https://www.postgresql.org/message-id/CANWNU8xm07jYUHxGh3XNHtcY37z%2B56-6bDD4piPt6%3DKidiHshQ%40mail.gmail.com ) However, I encountered an issue where cfbot did not apply the ".txt" file. I am still interested in learning **how to submit patches with the proper "text/x-patch" MIME type using Gmail.** I have noticed that some individuals have successfully used Gmail to submit patches with the correct MIME type. If anyone can provide assistance with this matter, I would be grateful. I am willing to contribute any helpful tips regarding using Gmail for patch submission to the Postgres wiki. I believe it is something like Mutt, right?
Re: Improve join_search_one_level readibilty (one line change)
On 04.06.23 08:02, Julien Rouhaud wrote: Additionally, if anyone has any tips on ensuring that Gmail recognizes my attached patches as the "text/x-patch" MIME type when sending them from the Chrome client, I would be grateful for the advice. I don't see any problem with the attachment. You can always check looking at the online archive for that, for instance for your email: https://www.postgresql.org/message-id/CANWNU8x9P9aCXGF=at-a_8mltat0lkcz_ysyrgbcuhzmqw2...@mail.gmail.com That shows exactly the problem being complained about.
Re: Improve join_search_one_level readibilty (one line change)
Thank you, Julien, for letting me know that cfbot doesn't test txt files. Much appreciated! v2-0001-Improve-left-deep-tree-dp-algorithm-s-readability.patch Description: Binary data
Re: Improve join_search_one_level readibilty (one line change)
Julien Rouhaud writes: > I'm glad I could help! Thanks for creating the cf entry. Note however that > the cfbot ignores files with a .txt extension (I don't think it's > documented but it will mostly handle files with diff, patch, gz(ip), tar > extensions IIRC, processing them as needed depending on the extension), Documented in the cfbot FAQ: https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches.3F which admittedly is a page a lot of people don't know about. regards, tom lane
Re: Improve join_search_one_level readibilty (one line change)
On Tue, 6 Jun 2023, 16:18 謝東霖, wrote: > Thank you to Julien Rouhaud and Tender Wang for the reviews. > > Julien's detailed guide has proven to be incredibly helpful, and I am > truly grateful for it. > Thank you so much for providing such valuable guidance! > > I have initiated a new commitfest: > https://commitfest.postgresql.org/43/4346/ > > Furthermore, I have attached a patch that improves the code by moving > the initialization of "other_rels_list" outside the if branching. > I'm glad I could help! Thanks for creating the cf entry. Note however that the cfbot ignores files with a .txt extension (I don't think it's documented but it will mostly handle files with diff, patch, gz(ip), tar extensions IIRC, processing them as needed depending on the extension), so you should send v2 again with a supported extension, otherwise the cfbot will keep testing your original patch. >
Re: Improve join_search_one_level readibilty (one line change)
Thank you to Julien Rouhaud and Tender Wang for the reviews.
Julien's detailed guide has proven to be incredibly helpful, and I am
truly grateful for it.
Thank you so much for providing such valuable guidance!
I have initiated a new commitfest:
https://commitfest.postgresql.org/43/4346/
Furthermore, I have attached a patch that improves the code by moving
the initialization of "other_rels_list" outside the if branching.
Perhaps Tom Lane would be interested in reviewing this minor change as well?
From 5b038577cf4f8acdc094d6e41d3891b559016cae Mon Sep 17 00:00:00 2001
From: DouEnergy
Date: Tue, 6 Jun 2023 15:30:14 +0800
Subject: [PATCH v2] Improve left deep tree dp algorithm's readability
---
src/backend/optimizer/path/joinrels.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/path/joinrels.c
b/src/backend/optimizer/path/joinrels.c
index 2feab2184f..a283e574f3 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -109,14 +109,14 @@ join_search_one_level(PlannerInfo *root, int level)
List *other_rels_list;
ListCell *other_rels;
+ other_rels_list = joinrels[1];
+
if (level == 2) /* consider remaining initial
rels */
{
- other_rels_list = joinrels[level - 1];
other_rels = lnext(other_rels_list, r);
}
else/* consider all initial
rels */
{
- other_rels_list = joinrels[1];
other_rels = list_head(other_rels_list);
}
--
2.37.1 (Apple Git-137.1)
Re: Improve join_search_one_level readibilty (one line change)
謝東霖 于2023年6月3日周六 23:21写道： > Hello hackers > > Attached is my first patch for PostgreSQL, which is a simple one-liner > that I believe can improve the code. > > In the "join_search_one_level" function, I noticed that the variable > "other_rels_list" always refers to "joinrels[1]" even when the (level > == 2) condition is met. I propose changing: > > "other_rels_list = joinrels[level - 1]" to "other_rels_list = joinrels[1]" > > This modification aims to enhance clarity and avoid unnecessary > instructions. > I guess compiler can make that code more efficiency. But from the point of code readibilty, I agree with you. As Julien Rouhaud said, it had better to to move the other_rels_list initialization out of the if instruction and put it with the variable declaration. I would greatly appreciate any review and feedback on this patch as I > am a newcomer to PostgreSQL contributions. Your input will help me > improve and contribute effectively to the project. > > I have followed the excellent guide "How to submit a patch by email, > 2023 edition" by Peter Eisentraut. > > Additionally, if anyone has any tips on ensuring that Gmail recognizes > my attached patches as the "text/x-patch" MIME type when sending them > from the Chrome client, I would be grateful for the advice. > > Or maybe the best practice is to use Git send-email ? > > Thank you for your time. > > Best regards > Alex Hsieh >
Re: Improve join_search_one_level readibilty (one line change)
Hi, On Sat, Jun 03, 2023 at 05:24:43PM +0800, 謝東霖 wrote: > > Attached is my first patch for PostgreSQL, which is a simple one-liner > that I believe can improve the code. Welcome! > In the "join_search_one_level" function, I noticed that the variable > "other_rels_list" always refers to "joinrels[1]" even when the (level > == 2) condition is met. I propose changing: > > "other_rels_list = joinrels[level - 1]" to "other_rels_list = joinrels[1]" > > This modification aims to enhance clarity and avoid unnecessary instructions. Agreed. It looks like it was originally introduced as mechanical changes in a bigger patch. It would probably be better to move the other_rels_list initialization out of the if instruction and put it with the variable declaration, to make it even clearer. I'm not that familiar with this area of the code so hopefully someone else will comment. > I would greatly appreciate any review and feedback on this patch as I > am a newcomer to PostgreSQL contributions. Your input will help me > improve and contribute effectively to the project. I think you did everything as needed! You should consider adding you patch to the next opened commitfest (https://commitfest.postgresql.org/43/) if you haven't already, to make sure it won't be forgotten, even if it's a one-liner. It will then also be regularly tested by the cfbot (http://cfbot.cputube.org/). If needed, you can also test the same CI jobs (covering multiple OS) using your personal github account, see https://github.com/postgres/postgres/blob/master/src/tools/ci/README on details to set it up. Best practice is also to review a patch of similar difficulty when sending one. You can look at the same commit fest entry if anything interests you, and register as a reviewer. > Additionally, if anyone has any tips on ensuring that Gmail recognizes > my attached patches as the "text/x-patch" MIME type when sending them > from the Chrome client, I would be grateful for the advice. I don't see any problem with the attachment. You can always check looking at the online archive for that, for instance for your email: https://www.postgresql.org/message-id/CANWNU8x9P9aCXGF=at-a_8mltat0lkcz_ysyrgbcuhzmqw2...@mail.gmail.com As far as I know only apple mail is problematic with that regards, as it doesn't send attachments as attachments. > Or maybe the best practice is to use Git send-email ? I don't think anyone uses git send-email on this mailing list. We usually prefer to manually attach patch(es), possibly compressing them if they're big, and then keep all the discussion and new revisions on the same thread.
Improve join_search_one_level readibilty (one line change)
Hello hackers
Attached is my first patch for PostgreSQL, which is a simple one-liner
that I believe can improve the code.
In the "join_search_one_level" function, I noticed that the variable
"other_rels_list" always refers to "joinrels[1]" even when the (level
== 2) condition is met. I propose changing:
"other_rels_list = joinrels[level - 1]" to "other_rels_list = joinrels[1]"
This modification aims to enhance clarity and avoid unnecessary instructions.
I would greatly appreciate any review and feedback on this patch as I
am a newcomer to PostgreSQL contributions. Your input will help me
improve and contribute effectively to the project.
I have followed the excellent guide "How to submit a patch by email,
2023 edition" by Peter Eisentraut.
Additionally, if anyone has any tips on ensuring that Gmail recognizes
my attached patches as the "text/x-patch" MIME type when sending them
from the Chrome client, I would be grateful for the advice.
Or maybe the best practice is to use Git send-email ?
Thank you for your time.
Best regards
Alex Hsieh
From 18d2ed81d3640881082bf37d6e3021ffe671d215 Mon Sep 17 00:00:00 2001
From: DouEnergy
Date: Fri, 2 Jun 2023 20:58:07 +0800
Subject: [PATCH] Improve left deep tree dp algorithm's readability
---
src/backend/optimizer/path/joinrels.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 2feab2184f..bca78d07fa 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -111,7 +111,7 @@ join_search_one_level(PlannerInfo *root, int level)
if (level == 2) /* consider remaining initial rels */
{
-other_rels_list = joinrels[level - 1];
+other_rels_list = joinrels[1];
other_rels = lnext(other_rels_list, r);
}
else/* consider all initial rels */
--
2.37.1 (Apple Git-137.1)
