Patch: clean up addRangeTableEntryForFunction

Started by David Fetteralmost 13 years ago6 messages

david@fetter.org

almost 13 years ago

1 attachment(s)

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

Attachments:

clean_up_addRangeTableEntryForFunction.patchtext/plain; charset=us-asciiDownload

*** a/src/backend/parser/parse_clause.c
--- b/src/backend/parser/parse_clause.c
***************
*** 536,543 **** transformRangeFunction(ParseState *pstate, RangeFunction *r)
  	/*
  	 * OK, build an RTE for the function.
  	 */
! 	rte = addRangeTableEntryForFunction(pstate, funcname, funcexpr,
! 										r, r->lateral, true);
  
  	/*
  	 * If a coldeflist was supplied, ensure it defines a legal set of names
--- 536,542 ----
  	/*
  	 * OK, build an RTE for the function.
  	 */
! 	rte = addRangeTableEntryForFunction(pstate, funcname, funcexpr, r, true);
  
  	/*
  	 * If a coldeflist was supplied, ensure it defines a legal set of names
*** a/src/backend/parser/parse_relation.c
--- b/src/backend/parser/parse_relation.c
***************
*** 1177,1183 **** addRangeTableEntryForFunction(ParseState *pstate,
  							  char *funcname,
  							  Node *funcexpr,
  							  RangeFunction *rangefunc,
- 							  bool lateral,
  							  bool inFromCl)
  {
  	RangeTblEntry *rte = makeNode(RangeTblEntry);
--- 1177,1182 ----
***************
*** 1285,1291 **** addRangeTableEntryForFunction(ParseState *pstate,
  	 * Functions are never checked for access rights (at least, not by the RTE
  	 * permissions mechanism).
  	 */
! 	rte->lateral = lateral;
  	rte->inh = false;			/* never true for functions */
  	rte->inFromCl = inFromCl;
  
--- 1284,1290 ----
  	 * Functions are never checked for access rights (at least, not by the RTE
  	 * permissions mechanism).
  	 */
! 	rte->lateral = rangefunc->lateral;
  	rte->inh = false;			/* never true for functions */
  	rte->inFromCl = inFromCl;
  
*** a/src/include/parser/parse_relation.h
--- b/src/include/parser/parse_relation.h
***************
*** 61,67 **** extern RangeTblEntry *addRangeTableEntryForFunction(ParseState *pstate,
  							  char *funcname,
  							  Node *funcexpr,
  							  RangeFunction *rangefunc,
- 							  bool lateral,
  							  bool inFromCl);
  extern RangeTblEntry *addRangeTableEntryForValues(ParseState *pstate,
  							List *exprs,
--- 61,66 ----

Craig Ringer

craig@2ndQuadrant.com

almost 13 years ago

In reply to: David Fetter (#1)

Re: Patch: clean up addRangeTableEntryForFunction

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

Tom Lane

tgl@sss.pgh.pa.us

almost 13 years ago

In reply to: David Fetter (#1)

Re: Patch: clean up addRangeTableEntryForFunction

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Fetter

david@fetter.org

almost 13 years ago

In reply to: Tom Lane (#3)

Re: Patch: clean up addRangeTableEntryForFunction

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?

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

over 12 years ago

In reply to: David Fetter (#1)

Re: Patch: clean up addRangeTableEntryForFunction

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 12 years ago

In reply to: Robert Haas (#5)

Re: Patch: clean up addRangeTableEntryForFunction

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers