Re: 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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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 01/23/2013 12:45 AM, David Fetter wrote:
> 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.
Added to CF 2013-next.

https://commitfest.postgresql.org/action/patch_view?id=1073

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

David Fetter <david(at)fetter(dot)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(at)fetter(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)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(at)fetter(dot)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(at)fetter(dot)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