Subject: Improve join_search_one_level readibilty (one line change)

From: 謝東霖 <[email-masked]>
Date: Sat, 3 Jun 2023 17:24:43 +0800

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: Julien Rouhaud <[email-masked]>
Date: Sun, 4 Jun 2023 14:02:49 +0800

Hi,

> that I believe can improve the code.

Welcome!

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

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

> 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=[email-masked]

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.

---

From: tender wang <[email-masked]>
Date: Tue, 6 Jun 2023 10:09:12 +0800

> This modification aims to enhance clarity and avoid unnecessary

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.

---

From: 謝東霖 <[email-masked]>
Date: Tue, 6 Jun 2023 16:18:30 +0800

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 <[email-masked]>
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)

---

From: Julien Rouhaud <[email-masked]>
Date: Wed, 7 Jun 2023 10:48:37 +0800

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

---

From: Tom Lane <[email-masked]>
Date: Tue, 06 Jun 2023 22:58:39 -0400

Julien Rouhaud <[email-masked]> writes:
> 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

---

From: 謝東霖 <[email-masked]>
Date: Wed, 7 Jun 2023 11:02:09 +0800

Thank you, Julien, for letting me know that cfbot doesn't test txt files.
Much appreciated!

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 7 Jun 2023 15:25:21 +0200

> https://www.postgresql.org/message-id/CANWNU8x9P9aCXGF=[email-masked]

That shows exactly the problem being complained about.

---

From: 謝東霖 <[email-masked]>
Date: Wed, 7 Jun 2023 23:05:19 +0800

Peter Eisentraut <[email-masked]>
> 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?

---

From: "Tristan Partin" <[email-masked]>
Date: Wed, 07 Jun 2023 10:26:59 -0500

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