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

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-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.

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?

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!

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-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." (
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?

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." (
> 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)

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

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