Thread: 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
Attachment
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 -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 Thanks for fixing, Tom! James