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
Attachments:
0001-Improve-left-deep-tree-dp-algorithm-s-readability.patchapplication/x-patch; name=0001-Improve-left-deep-tree-dp-algorithm-s-readability.patchDownload
From 18d2ed81d3640881082bf37d6e3021ffe671d215 Mon Sep 17 00:00:00 2001
From: DouEnergy <douenergy@protonmail.com>
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)
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:
/messages/by-id/CANWNU8x9P9aCXGF=aT-A_8mLTAT0LkcZ_ySYrGbcuHzMQw2-1g@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.
謝東霖 <douenergy@gmail.com> 于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
Show quoted text
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
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?
Attachments:
v2-0001-Improve-left-deep-tree-dp-algorithm-s-readability.txttext/plain; charset=US-ASCII; name=v2-0001-Improve-left-deep-tree-dp-algorithm-s-readability.txtDownload
From 5b038577cf4f8acdc094d6e41d3891b559016cae Mon Sep 17 00:00:00 2001
From: DouEnergy <douenergy@protonmail.com>
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)
On Tue, 6 Jun 2023, 16:18 謝東霖, <douenergy@gmail.com> 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.
Show quoted text
Julien Rouhaud <rjuju123@gmail.com> 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
Thank you, Julien, for letting me know that cfbot doesn't test txt files.
Much appreciated!
Attachments:
v2-0001-Improve-left-deep-tree-dp-algorithm-s-readability.patchapplication/octet-stream; name=v2-0001-Improve-left-deep-tree-dp-algorithm-s-readability.patchDownload
From 5b038577cf4f8acdc094d6e41d3891b559016cae Mon Sep 17 00:00:00 2001
From: DouEnergy <douenergy@protonmail.com>
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)
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:
/messages/by-id/CANWNU8x9P9aCXGF=aT-A_8mLTAT0LkcZ_ySYrGbcuHzMQw2-1g@mail.gmail.com
That shows exactly the problem being complained about.
Peter Eisentraut <peter@eisentraut.org>
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." (
/messages/by-id/CANWNU8xm07jYUHxGh3XNHtcY37z+56-6bDD4piPt6=KidiHshQ@mail.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?
On Wed Jun 7, 2023 at 10:05 AM CDT, 謝東霖 wrote:
Peter Eisentraut <peter@eisentraut.org>
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." (
/messages/by-id/CANWNU8xm07jYUHxGh3XNHtcY37z+56-6bDD4piPt6=KidiHshQ@mail.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)
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.
On Tue, 1 Aug 2023 at 01:48, Julien Rouhaud <rjuju123@gmail.com> 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
Attachments:
join_search_one_level_tidyup.patchapplication/octet-stream; name=join_search_one_level_tidyup.patchDownload
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 015a0b3cbe..235c26d200 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -25,8 +25,8 @@
static void make_rels_by_clause_joins(PlannerInfo *root,
RelOptInfo *old_rel,
- List *other_rels_list,
- ListCell *other_rels);
+ List *other_rels,
+ int first_rel_idx);
static void make_rels_by_clauseless_joins(PlannerInfo *root,
RelOptInfo *old_rel,
List *other_rels);
@@ -93,6 +93,8 @@ join_search_one_level(PlannerInfo *root, int level)
if (old_rel->joininfo != NIL || old_rel->has_eclass_joins ||
has_join_restriction(root, old_rel))
{
+ int first_rel;
+
/*
* There are join clauses or join order restrictions relevant to
* this rel, so consider joins between this rel and (only) those
@@ -106,24 +108,12 @@ join_search_one_level(PlannerInfo *root, int level)
* to each initial rel they don't already include but have a join
* clause or restriction with.
*/
- List *other_rels_list;
- ListCell *other_rels;
-
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);
- }
+ first_rel = foreach_current_index(r) + 1;
+ else
+ first_rel = 0;
- make_rels_by_clause_joins(root,
- old_rel,
- other_rels_list,
- other_rels);
+ make_rels_by_clause_joins(root, old_rel, joinrels[1], first_rel);
}
else
{
@@ -286,9 +276,8 @@ join_search_one_level(PlannerInfo *root, int level)
* automatically ensures that each new joinrel is only added to the list once.
*
* 'old_rel' is the relation entry for the relation to be joined
- * 'other_rels_list': a list containing the other
- * rels to be considered for joining
- * 'other_rels': the first cell to be considered
+ * 'other_rels': a list containing the other rels to be considered for joining
+ * 'first_rel_idx': the first rel to be considered in 'other_rels'
*
* Currently, this is only used with initial rels in other_rels, but it
* will work for joining to joinrels too.
@@ -296,12 +285,12 @@ join_search_one_level(PlannerInfo *root, int level)
static void
make_rels_by_clause_joins(PlannerInfo *root,
RelOptInfo *old_rel,
- List *other_rels_list,
- ListCell *other_rels)
+ List *other_rels,
+ int first_rel_idx)
{
ListCell *l;
- for_each_cell(l, other_rels_list, other_rels)
+ for_each_from(l, other_rels, first_rel_idx)
{
RelOptInfo *other_rel = (RelOptInfo *) lfirst(l);
On Fri, Aug 4, 2023 at 10:36 AM David Rowley <dgrowleyml@gmail.com> 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
Attachments:
v2-0001-join_search_one_level-tidyup.patchapplication/octet-stream; name=v2-0001-join_search_one_level-tidyup.patchDownload
From f240e35134aa1fa1f7e24e07ff6c8cdcdf85d81c Mon Sep 17 00:00:00 2001
From: pgsql-guo <richard.guo@openpie.com>
Date: Fri, 4 Aug 2023 11:25:23 +0800
Subject: [PATCH v2] join_search_one_level tidyup
---
src/backend/optimizer/path/joinrels.c | 55 +++++++++------------------
1 file changed, 18 insertions(+), 37 deletions(-)
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
index 015a0b3cbe..5a1e8ba73d 100644
--- a/src/backend/optimizer/path/joinrels.c
+++ b/src/backend/optimizer/path/joinrels.c
@@ -25,8 +25,8 @@
static void make_rels_by_clause_joins(PlannerInfo *root,
RelOptInfo *old_rel,
- List *other_rels_list,
- ListCell *other_rels);
+ List *other_rels,
+ int first_rel_idx);
static void make_rels_by_clauseless_joins(PlannerInfo *root,
RelOptInfo *old_rel,
List *other_rels);
@@ -93,6 +93,8 @@ join_search_one_level(PlannerInfo *root, int level)
if (old_rel->joininfo != NIL || old_rel->has_eclass_joins ||
has_join_restriction(root, old_rel))
{
+ int first_rel;
+
/*
* There are join clauses or join order restrictions relevant to
* this rel, so consider joins between this rel and (only) those
@@ -106,24 +108,12 @@ join_search_one_level(PlannerInfo *root, int level)
* to each initial rel they don't already include but have a join
* clause or restriction with.
*/
- List *other_rels_list;
- ListCell *other_rels;
-
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);
- }
+ first_rel = foreach_current_index(r) + 1;
+ else
+ first_rel = 0;
- make_rels_by_clause_joins(root,
- old_rel,
- other_rels_list,
- other_rels);
+ make_rels_by_clause_joins(root, old_rel, joinrels[1], first_rel);
}
else
{
@@ -167,8 +157,7 @@ join_search_one_level(PlannerInfo *root, int level)
foreach(r, joinrels[k])
{
RelOptInfo *old_rel = (RelOptInfo *) lfirst(r);
- List *other_rels_list;
- ListCell *other_rels;
+ int first_rel;
ListCell *r2;
/*
@@ -180,19 +169,12 @@ join_search_one_level(PlannerInfo *root, int level)
!has_join_restriction(root, old_rel))
continue;
- if (k == other_level)
- {
- /* only consider remaining rels */
- other_rels_list = joinrels[k];
- other_rels = lnext(other_rels_list, r);
- }
+ if (k == other_level) /* only consider remaining rels */
+ first_rel = foreach_current_index(r) + 1;
else
- {
- other_rels_list = joinrels[other_level];
- other_rels = list_head(other_rels_list);
- }
+ first_rel = 0;
- for_each_cell(r2, other_rels_list, other_rels)
+ for_each_from(r2, joinrels[other_level], first_rel)
{
RelOptInfo *new_rel = (RelOptInfo *) lfirst(r2);
@@ -286,9 +268,8 @@ join_search_one_level(PlannerInfo *root, int level)
* automatically ensures that each new joinrel is only added to the list once.
*
* 'old_rel' is the relation entry for the relation to be joined
- * 'other_rels_list': a list containing the other
- * rels to be considered for joining
- * 'other_rels': the first cell to be considered
+ * 'other_rels': a list containing the other rels to be considered for joining
+ * 'first_rel_idx': the first rel to be considered in 'other_rels'
*
* Currently, this is only used with initial rels in other_rels, but it
* will work for joining to joinrels too.
@@ -296,12 +277,12 @@ join_search_one_level(PlannerInfo *root, int level)
static void
make_rels_by_clause_joins(PlannerInfo *root,
RelOptInfo *old_rel,
- List *other_rels_list,
- ListCell *other_rels)
+ List *other_rels,
+ int first_rel_idx)
{
ListCell *l;
- for_each_cell(l, other_rels_list, other_rels)
+ for_each_from(l, other_rels, first_rel_idx)
{
RelOptInfo *other_rel = (RelOptInfo *) lfirst(l);
--
2.32.0
On Fri, 4 Aug 2023 at 16:05, Richard Guo <guofenglinux@gmail.com> wrote:
On Fri, Aug 4, 2023 at 10:36 AM David Rowley <dgrowleyml@gmail.com> 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