Subject: Re: Improve join_search_one_level readibilty (one line change)

From: Julien Rouhaud <[email-masked]>
Date: Mon, 31 Jul 2023 21:48:37 +0800

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.

---

From: David Rowley <[email-masked]>
Date: Fri, 4 Aug 2023 14:36:15 +1200

> 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

---

From: Richard Guo <[email-masked]>
Date: Fri, 4 Aug 2023 12:05:14 +0800

> 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

---

From: David Rowley <[email-masked]>
Date: Sun, 6 Aug 2023 21:55:18 +1200

> I think we can go even further to do the same for 'bushy plans' case,

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