Possible typo/unclear comment in joinpath.c
In joinpath.c three times we reference "extra_lateral_rels" (with
underscores like it's a field), but as far as I can tell that's not a
field anywhere in the source code, and looking at the code that
follows it seems like it should be referencing "lateral_relids" (and
the "extra" is really "extra [in relation to relids]").
Assuming that interpretation is correct, I'd attached a patch to
change all three occurrences to "extra lateral_relids" to reduce
confusion.
Thanks,
James
Attachments:
v1-0001-Fix-extra_lateral_rels-typo.patchapplication/octet-stream; name=v1-0001-Fix-extra_lateral_rels-typo.patchDownload
From 633b02897fc8a279fb4721b86fd9157395b09431 Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Wed, 14 Apr 2021 14:20:12 +0000
Subject: [PATCH v1] Fix extra_lateral_rels typo
---
src/backend/optimizer/path/joinpath.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index e9b6968b1d..d9fc3621ba 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -671,7 +671,7 @@ try_partial_nestloop_path(PlannerInfo *root,
* If the inner path is parameterized, the parameterization must be fully
* satisfied by the proposed outer path. Parameterized partial paths are
* not supported. The caller should already have verified that no
- * extra_lateral_rels are required here.
+ * extra lateral_relids are required here.
*/
Assert(bms_is_empty(joinrel->lateral_relids));
if (inner_path->param_info != NULL)
@@ -985,7 +985,7 @@ try_partial_hashjoin_path(PlannerInfo *root,
* If the inner path is parameterized, the parameterization must be fully
* satisfied by the proposed outer path. Parameterized partial paths are
* not supported. The caller should already have verified that no
- * extra_lateral_rels are required here.
+ * extra lateral_relids are required here.
*/
Assert(bms_is_empty(joinrel->lateral_relids));
if (inner_path->param_info != NULL)
@@ -1714,7 +1714,7 @@ match_unsorted_outer(PlannerInfo *root,
* partial path and the joinrel is parallel-safe. However, we can't
* handle JOIN_UNIQUE_OUTER, because the outer path will be partial, and
* therefore we won't be able to properly guarantee uniqueness. Nor can
- * we handle extra_lateral_rels, since partial paths must not be
+ * we handle extra lateral_relids, since partial paths must not be
* parameterized. Similarly, we can't handle JOIN_FULL and JOIN_RIGHT,
* because they can produce false null extended rows.
*/
--
2.20.1
On Wed, Apr 14, 2021 at 11:36:38AM -0400, James Coleman wrote:
In joinpath.c three times we reference "extra_lateral_rels" (with
underscores like it's a field), but as far as I can tell that's not a
field anywhere in the source code, and looking at the code that
follows it seems like it should be referencing "lateral_relids" (and
the "extra" is really "extra [in relation to relids]").
It looks like a loose end from
commit edca44b1525b3d591263d032dc4fe500ea771e0e
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon Dec 7 18:56:14 2015 -0500
Simplify LATERAL-related calculations within add_paths_to_joinrel().
--
Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
On Wed, Apr 14, 2021 at 11:36:38AM -0400, James Coleman wrote:
In joinpath.c three times we reference "extra_lateral_rels" (with
underscores like it's a field), but as far as I can tell that's not a
field anywhere in the source code, and looking at the code that
follows it seems like it should be referencing "lateral_relids" (and
the "extra" is really "extra [in relation to relids]").
It looks like a loose end from
commit edca44b1525b3d591263d032dc4fe500ea771e0e
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon Dec 7 18:56:14 2015 -0500
Simplify LATERAL-related calculations within add_paths_to_joinrel().
Yeah :-(. I'm usually pretty careful about grepping for comment
references as well as code references to a field when I do something
like that, but obviously I missed that step that time.
Will fix, thanks James!
regards, tom lane
I wrote:
Justin Pryzby <pryzby@telsasoft.com> writes:
It looks like a loose end from
commit edca44b1525b3d591263d032dc4fe500ea771e0e
Yeah :-(. I'm usually pretty careful about grepping for comment
references as well as code references to a field when I do something
like that, but obviously I missed that step that time.
No, I take that back. There were no references to extra_lateral_rels
after that commit; these comments were added by 45be99f8c, about
six weeks later. The latter was a pretty large patch and had
presumably been under development for quite some time, so the comments
were probably accurate when written but didn't get updated.
regards, tom lane
On Wed, Apr 14, 2021 at 2:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
No, I take that back. There were no references to extra_lateral_rels
after that commit; these comments were added by 45be99f8c, about
six weeks later. The latter was a pretty large patch and had
presumably been under development for quite some time, so the comments
were probably accurate when written but didn't get updated.
Woops.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, Apr 14, 2021 at 1:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Justin Pryzby <pryzby@telsasoft.com> writes:
On Wed, Apr 14, 2021 at 11:36:38AM -0400, James Coleman wrote:
In joinpath.c three times we reference "extra_lateral_rels" (with
underscores like it's a field), but as far as I can tell that's not a
field anywhere in the source code, and looking at the code that
follows it seems like it should be referencing "lateral_relids" (and
the "extra" is really "extra [in relation to relids]").It looks like a loose end from
commit edca44b1525b3d591263d032dc4fe500ea771e0e
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon Dec 7 18:56:14 2015 -0500Simplify LATERAL-related calculations within add_paths_to_joinrel().
Yeah :-(. I'm usually pretty careful about grepping for comment
references as well as code references to a field when I do something
like that, but obviously I missed that step that time.Will fix, thanks James!
regards, tom lane
Thanks for fixing, Tom!
James