Thread: Patch: clean up addRangeTableEntryForFunction
Folks, I've been working with Andrew Gierth (well, mostly he's been doing the work, as usual) to add WITH ORDINALITY as an option for set-returning functions. In the process, he found a minor opportunity to clean up the interface for $SUBJECT, reducing the call to a Single Point of Truth for lateral-ness, very likely improving the efficiency of calls to that function. Please find attached the patch. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachment
<div class="moz-cite-prefix">On 01/23/2013 12:45 AM, David Fetter wrote:<br /></div><blockquote cite="mid:20130122164512.GA16248@fetter.org"type="cite"><pre wrap="">Folks, I've been working with Andrew Gierth (well, mostly he's been doing the work, as usual) to add WITH ORDINALITY as an option for set-returning functions. In the process, he found a minor opportunity to clean up the interface for $SUBJECT, reducing the call to a Single Point of Truth for lateral-ness, very likely improving the efficiency of calls to that function.</pre></blockquote> Added to CF 2013-next.<br /><br /><a href="https://commitfest.postgresql.org/action/patch_view?id=1073">https://commitfest.postgresql.org/action/patch_view?id=1073</a><br /><br/><pre class="moz-signature" cols="72">-- Craig Ringer <a class="moz-txt-link-freetext" href="http://www.2ndQuadrant.com/">http://www.2ndQuadrant.com/</a>PostgreSQLDevelopment, 24x7 Support, Training & Services</pre>
David Fetter <david@fetter.org> writes:
> I've been working with Andrew Gierth (well, mostly he's been doing the
> work, as usual) to add WITH ORDINALITY as an option for set-returning
> functions. In the process, he found a minor opportunity to clean up
> the interface for $SUBJECT, reducing the call to a Single Point of
> Truth for lateral-ness, very likely improving the efficiency of calls
> to that function.
As I mentioned in our off-list discussion, I think this is going in the
wrong direction. It'd make more sense to me to get rid of the
RangeFunction parameter, instead passing the two fields that
addRangeTableEntryForFunction actually uses out of that. If we do what
you have here, we'll be welding together the alias and lateral settings
for the new RTE; which conceivably some other caller would want to
specify in a different way. As a comparison point, you might want to
look at the various calls to addRangeTableEntryForSubquery: some of
those pass multiple fields out of the same RangeSubselect, and some
do not.
regards, tom lane
On Tue, Jan 22, 2013 at 11:02:18PM -0500, Tom Lane wrote: > David Fetter <david@fetter.org> writes: > > I've been working with Andrew Gierth (well, mostly he's been doing > > the work, as usual) to add WITH ORDINALITY as an option for > > set-returning functions. In the process, he found a minor > > opportunity to clean up the interface for $SUBJECT, reducing the > > call to a Single Point of Truth for lateral-ness, very likely > > improving the efficiency of calls to that function. > > As I mentioned in our off-list discussion, I think this is going in > the wrong direction. It'd make more sense to me to get rid of the > RangeFunction parameter, instead passing the two fields that > addRangeTableEntryForFunction actually uses out of that. With utmost respect, of the four fields currently in RangeFunction: type (tag), lateral, funccallnode, alias, and coldeflist, the function needs three (all but funccallnode, which has already been transformed into a funcexpr). The patch for ordinality makes that 4/5 with the ordinality field added. > If we do what you have here, we'll be welding together the alias and > lateral settings for the new RTE; which conceivably some other > caller would want to specify in a different way. As a comparison > point, you might want to look at the various calls to > addRangeTableEntryForSubquery: some of those pass multiple fields > out of the same RangeSubselect, and some do not. As to addRangeTableEntryForSubquery, I'm not seeing the connection to the case at hand. Could you please spell it out? Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Tue, Jan 22, 2013 at 11:45 AM, David Fetter <david@fetter.org> wrote: > I've been working with Andrew Gierth (well, mostly he's been doing the > work, as usual) to add WITH ORDINALITY as an option for set-returning > functions. In the process, he found a minor opportunity to clean up > the interface for $SUBJECT, reducing the call to a Single Point of > Truth for lateral-ness, very likely improving the efficiency of calls > to that function. > > Please find attached the patch. I think this patch is utterly pointless. I recommend we reject it. If this were part of some larger refactoring that was going in some direction we could agree on, it might be worth it. But as it is, I think it's just a shot in the dark whether this change will end up being better or worse, and my personal bet is that it won't make any difference whatsoever. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Tue, Jan 22, 2013 at 11:45 AM, David Fetter <david@fetter.org> wrote: > > I've been working with Andrew Gierth (well, mostly he's been doing the > > work, as usual) to add WITH ORDINALITY as an option for set-returning > > functions. In the process, he found a minor opportunity to clean up > > the interface for $SUBJECT, reducing the call to a Single Point of > > Truth for lateral-ness, very likely improving the efficiency of calls > > to that function. > > > > Please find attached the patch. > > I think this patch is utterly pointless. I recommend we reject it. > If this were part of some larger refactoring that was going in some direction > we could agree on, it might be worth it. But as it is, I think it's just a > shot in the dark whether this change will end up being better or worse, and > my personal bet is that it won't make any difference whatsoever. To be frank, I agree with Robert. Sorry for the delay in my review. Best regards, Etsuro Fujita