From: | James Coleman <jtc331(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Fix incorrect comment reference |
Date: | 2023-01-23 21:07:03 |
Message-ID: | CAAaqYe_u8ELCr1raCNbFLd=cUo69cS7kUmrchDGtSKYM2OJtXA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Jan 23, 2023 at 3:41 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Jan 23, 2023 at 3:19 PM James Coleman <jtc331(at)gmail(dot)com> wrote:
> > On Mon, Jan 23, 2023 at 1:26 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > On Mon, Jan 23, 2023 at 8:31 AM James Coleman <jtc331(at)gmail(dot)com> wrote:
> > > > See the attached for a simple comment fix -- the referenced
> > > > generate_useful_gather_paths call isn't in grouping_planner it's in
> > > > apply_scanjoin_target_to_paths.
> > >
> > > The intended reading of the comment is not clear. Is it telling you to
> > > look at grouping_planner because that's where we
> > > generate_useful_gather_paths, or is it telling you to look there to
> > > see how we get the final target list together? If it's the former,
> > > then your fix is correct. If the latter, it's fine as it is.
> > >
> > > The real answer is probably that some years ago both things happened
> > > in that function. We've moved on from there, but I'm still not sure
> > > what the most useful phrasing of the comment is.
> >
> > Yeah, almost certainly, and the comments just didn't keep up.
> >
> > Would you prefer something that notes both that the broader concern is
> > happening via the grouping_planner() stage but still points to the
> > proper callsite (so that people don't go looking for that confused)?
>
> I don't really have a strong view on what the best thing to do is. I
> was just pointing out that the comment might not be quite so obviously
> wrong as you were supposing.
"Wrong" is certainly too strong; my apologies.
I'm really just hoping to improve it for future readers to save them
some confusion I had initially reading it.
James Coleman
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Luzanov | 2023-01-23 21:07:58 | Re: v16 GRANT role TO role needs a multi-option setting capability |
Previous Message | Jelte Fennema | 2023-01-23 20:53:00 | Re: run pgindent on a regular basis / scripted manner |