Horton

Thread

Commits

  1. Remove obsoleted code relating to targetlist SRF evaluation.

  2. Doc: improve documentation of new SRF-in-tlist behavior.

  3. Move targetlist SRF handling from expression evaluation to new executor node.

  4. Don't split up SRFs when choosing to postpone SELECT output expressions.

  1. Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-05-23T00:53:27Z

    Hi,

discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in the target
list.

One idea I circulated was to fix that by interjecting a special executor
node to process SRF containing targetlists (reusing Result possibly?).
That'd allow to remove the isDone argument from ExecEval*/ExecProject*
and get rid of ps_TupFromTlist which is fairly ugly.


Robert suggested - IIRC mentioning previous on-list discussion - to
instead rewrite targetlist SRFs into lateral joins. My gut feeling is
that that'd be a larger undertaking, with significant semantics changes.

If we accept bigger semantical changes, I'm inclined to instead just get
rid of targetlist SRFs in total; they're really weird and not needed
anymore.

One issue with removing targetlist SRFs is that they're currently
considerably faster than SRFs in FROM:
tpch[14693][1]=# COPY (SELECT * FROM generate_series(1, 10000000)) TO '/dev/null';
COPY 10000000
Time: 2217.167 ms
tpch[14693][1]=# COPY (SELECT generate_series(1, 10000000)) TO '/dev/null';
COPY 10000000
Time: 1355.929 ms
tpch[14693][1]=#

I'm no tto concerned about that, and we could probably fixing by
removing forced materialization from the relevant code path.

Comments?

Greetings,

Andres Freund

  2. Re: Changed SRF in targetlist handling

    Craig Ringer <craig@2ndquadrant.com> — 2016-05-23T01:26:03Z

    On 23 May 2016 at 08:53, Andres Freund <andres@anarazel.de> wrote:

> Hi,
>
> discussing executor performance with a number of people at pgcon,
> several hackers - me included - complained about the additional
> complexity, both code and runtime, required to handle SRFs in the target
> list.
>
> One idea I circulated was to fix that by interjecting a special executor
> node to process SRF containing targetlists (reusing Result possibly?).
> That'd allow to remove the isDone argument from ExecEval*/ExecProject*
> and get rid of ps_TupFromTlist which is fairly ugly.
>
>
> Robert suggested - IIRC mentioning previous on-list discussion - to
> instead rewrite targetlist SRFs into lateral joins. My gut feeling is
> that that'd be a larger undertaking, with significant semantics changes.
>
> If we accept bigger semantical changes, I'm inclined to instead just get
> rid of targetlist SRFs in total; they're really weird and not needed
> anymore.
>
> One issue with removing targetlist SRFs is that they're currently
> considerably faster than SRFs in FROM:
> tpch[14693][1]=# COPY (SELECT * FROM generate_series(1, 10000000)) TO
> '/dev/null';
> COPY 10000000
> Time: 2217.167 ms
> tpch[14693][1]=# COPY (SELECT generate_series(1, 10000000)) TO '/dev/null';
> COPY 10000000
> Time: 1355.929 ms
> tpch[14693][1]=#
>
> I'm no tto concerned about that, and we could probably fixing by
> removing forced materialization from the relevant code path.
>
> Comments?
>
>
SRFs-in-tlist are a lot faster for lockstep iteration etc. They're also
much simpler to write, though if the result result rowcount differs
unexpectedly between the functions you get exciting and unexpected
behaviour.

WITH ORDINALITY provides what I think is the last of the functionality
needed to replace SRFs-in-from, but at a syntatactic complexity and
performance cost. The following example demonstrates that, though it
doesn't do anything that needs LATERAL etc. I'm aware the following aren't
semantically identical if the rowcounts differ.


craig=> EXPLAIN ANALYZE SELECT generate_series(1,1000000) x,
generate_series(1,1000000) y;
                                          QUERY PLAN

----------------------------------------------------------------------------------------------
 Result  (cost=0.00..5.01 rows=1000 width=0) (actual time=0.024..92.845
rows=1000000 loops=1)
 Planning time: 0.039 ms
 Execution time: 123.123 ms
(3 rows)

Time: 123.719 ms


craig=> EXPLAIN ANALYZE SELECT x, y FROM generate_series(1,1000000) WITH
ORDINALITY AS x(i, n) INNER JOIN generate_series(1,1000000) WITH ORDINALITY
AS y(i, n) ON (x.n = y.n);
                                                                QUERY PLAN

------------------------------------------------------------------------------------------------------------------------------------------
 Merge Join  (cost=0.01..97.50 rows=5000 width=64) (actual
time=179.863..938.375 rows=1000000 loops=1)
   Merge Cond: (x.n = y.n)
   ->  Function Scan on generate_series x  (cost=0.00..10.00 rows=1000
width=40) (actual time=108.813..303.690 rows=1000000 loops=1)
   ->  Materialize  (cost=0.00..12.50 rows=1000 width=40) (actual
time=71.043..372.880 rows=1000000 loops=1)
         ->  Function Scan on generate_series y  (cost=0.00..10.00
rows=1000 width=40) (actual time=71.039..266.209 rows=1000000 loops=1)
 Planning time: 0.184 ms
 Execution time: 970.744 ms
(7 rows)

Time: 971.706 ms


I get the impression the with-ordinality case could perform just as well if
the optimiser recognised a join on the ordinality column and iterated the
functions in lockstep to populate the result row directly. Though that
could perform _worse_ if the function is computationally costly and
benefits significantly from the CPU cache, where we're better off
materializing it or at least executing it in chunks/batches...


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

  3. Re: Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-05-23T02:09:25Z

    tl;dr

Semantic changes to SRF-in-target-list processing are undesirable when they
are all but deprecated.

I'd accept a refactoring that trades a performance gain for unaffected
queries for a reasonable performance hit of those afflicted.

Preamble...

Most recent thread that I can recall seeing on the topic - and where I
believe the rewrite idea was first presented.

http://www.postgresql.org/message-id/flat/25750.1458767514@sss.pgh.pa.us#25750.1458767514@sss.pgh.pa.us

On Sun, May 22, 2016 at 8:53 PM, Andres Freund <andres@anarazel.de> wrote:

> Hi,
>
> discussing executor performance with a number of people at pgcon,
> several hackers - me included - complained about the additional
> complexity, both code and runtime, required to handle SRFs in the target
> list.
>
> One idea I circulated was to fix that by interjecting a special executor
> node to process SRF containing targetlists (reusing Result possibly?).
> That'd allow to remove the isDone argument from ExecEval*/ExecProject*
> and get rid of ps_TupFromTlist which is fairly ugly.
>

​Conceptually I'm all for minimizing the impact on queries of this form.
It seems to be the most likely to get written and committed and the least
likely to cause unforeseen issues.
​


> Robert suggested - IIRC mentioning previous on-list discussion - to
> instead rewrite targetlist SRFs into lateral joins. My gut feeling is
> that that'd be a larger undertaking, with significant semantics changes.
>
​[...]​

> If we accept bigger semantical changes, I'm inclined to instead just get
> rid of targetlist SRFs in total; they're really weird and not needed
> anymore.
>

​I cannot see these, in isolation, being a good option.  Nonetheless, I
don't think any semantic change should happen before 9.2 becomes no longer
supported.  I'd be inclined to take a similar approach as with
standard_conforming_strings (minus the execution guc, just the warning one)
with whatever after-the-fact learning taken into account.

Its worth considering query rewrite and making it forbidden as a joint goal.

For something like a canonical version of this, especially for
composite-returning SRF:

WITH func_call (
SELECT func(tbl.col)
FROM tbl
)
​SELECT (func_call.func).*
FROM func_call;​

If we can rewrite the CTE portion into a lateral - with the exact same
semantics (specifically, returning the single-column composite) then check
the rewritten query the select list SRF would not longer be present and no
error would be thrown.

For situations where a rewrite cannot be made to behave properly we leave
the construct alone and let the query raise an error.

In considering what I just wrote I'm not particularly enamored with
it...hence my overall conclusion.  Can't say I hate it and after re-reading
the aforementioned thread I'm inclined to like it for cases where, for
instance, we are susceptible to a LCM evaluation.

David J.

  4. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-23T17:10:29Z

    Andres Freund <andres@anarazel.de> writes:
> discussing executor performance with a number of people at pgcon,
> several hackers - me included - complained about the additional
> complexity, both code and runtime, required to handle SRFs in the target
> list.

Yeah, this has been an annoyance for a long time.

> One idea I circulated was to fix that by interjecting a special executor
> node to process SRF containing targetlists (reusing Result possibly?).
> That'd allow to remove the isDone argument from ExecEval*/ExecProject*
> and get rid of ps_TupFromTlist which is fairly ugly.

Would that not lead to, in effect, duplicating all of execQual.c?  The new
executor node would still have to be prepared to process all expression
node types.

> Robert suggested - IIRC mentioning previous on-list discussion - to
> instead rewrite targetlist SRFs into lateral joins. My gut feeling is
> that that'd be a larger undertaking, with significant semantics changes.

Yes, this was discussed on-list awhile back (I see David found a reference
already).  I think it's feasible, although we'd first have to agree
whether we want to remain bug-compatible with the old
least-common-multiple-of-the-periods behavior.  I would vote for not,
but it's certainly a debatable thing.

> If we accept bigger semantical changes, I'm inclined to instead just get
> rid of targetlist SRFs in total; they're really weird and not needed
> anymore.

This seems a bridge too far to me.  It's just way too common to do
"select generate_series(1,n)".  We could tell people they have to
rewrite to "select * from generate_series(1,n)", but it would be far
more polite to do that for them.

> One issue with removing targetlist SRFs is that they're currently
> considerably faster than SRFs in FROM:

I suspect that depends greatly on your test case.  But in any case
we could put more effort into optimizing nodeFunctionscan.

			regards, tom lane

  5. Re: Changed SRF in targetlist handling

    David Fetter <david@fetter.org> — 2016-05-23T17:35:10Z

    On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote:
> This seems a bridge too far to me.  It's just way too common to do
> "select generate_series(1,n)".  We could tell people they have to
> rewrite to "select * from generate_series(1,n)", but it would be far
> more polite to do that for them.

How about making "TABLE generate_series(1,n)" work?  It's even
shorter in exchange for some cognitive load.

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

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

  6. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-23T17:36:57Z

    David Fetter <david@fetter.org> writes:
> On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote:
>> This seems a bridge too far to me.  It's just way too common to do
>> "select generate_series(1,n)".  We could tell people they have to
>> rewrite to "select * from generate_series(1,n)", but it would be far
>> more polite to do that for them.

> How about making "TABLE generate_series(1,n)" work?  It's even
> shorter in exchange for some cognitive load.

No thanks --- the word after TABLE ought to be a table name, not some
arbitrary expression.  That's way too much mess to save one keystroke.

			regards, tom lane

  7. Re: Changed SRF in targetlist handling

    David Fetter <david@fetter.org> — 2016-05-23T17:44:20Z

    On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote:
> David Fetter <david@fetter.org> writes:
> > On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote:
> >> This seems a bridge too far to me.  It's just way too common to do
> >> "select generate_series(1,n)".  We could tell people they have to
> >> rewrite to "select * from generate_series(1,n)", but it would be far
> >> more polite to do that for them.
> 
> > How about making "TABLE generate_series(1,n)" work?  It's even
> > shorter in exchange for some cognitive load.
> 
> No thanks --- the word after TABLE ought to be a table name, not some
> arbitrary expression.  That's way too much mess to save one keystroke.

It's not just about saving a keystroke.  This change would go with
removing the ability to do SRFs in the target list of a SELECT
query.

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

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

  8. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-23T18:27:23Z

    David Fetter <david@fetter.org> writes:
> On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote:
>> David Fetter <david@fetter.org> writes:
>>> How about making "TABLE generate_series(1,n)" work?  It's even
>>> shorter in exchange for some cognitive load.

>> No thanks --- the word after TABLE ought to be a table name, not some
>> arbitrary expression.  That's way too much mess to save one keystroke.

> It's not just about saving a keystroke.  This change would go with
> removing the ability to do SRFs in the target list of a SELECT
> query.

I guess you did not understand that I was rejecting doing that.
Telling people they have to modify existing code that does this and
works fine is exactly what I felt we can't do.  We might be able to
blow off complicated cases, but I think simpler cases are too common
in the field.

I'm on board with fixing things so that the *implementation* doesn't
support SRF-in-tlist.  But we can't just remove it from the language.

			regards, tom lane

  9. Re: Changed SRF in targetlist handling

    Merlin Moncure <mmoncure@gmail.com> — 2016-05-23T18:28:11Z

    On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> discussing executor performance with a number of people at pgcon,
>> several hackers - me included - complained about the additional
>> complexity, both code and runtime, required to handle SRFs in the target
>> list.
>
> Yeah, this has been an annoyance for a long time.
>
>> One idea I circulated was to fix that by interjecting a special executor
>> node to process SRF containing targetlists (reusing Result possibly?).
>> That'd allow to remove the isDone argument from ExecEval*/ExecProject*
>> and get rid of ps_TupFromTlist which is fairly ugly.
>
> Would that not lead to, in effect, duplicating all of execQual.c?  The new
> executor node would still have to be prepared to process all expression
> node types.
>
>> Robert suggested - IIRC mentioning previous on-list discussion - to
>> instead rewrite targetlist SRFs into lateral joins. My gut feeling is
>> that that'd be a larger undertaking, with significant semantics changes.
>
> Yes, this was discussed on-list awhile back (I see David found a reference
> already).  I think it's feasible, although we'd first have to agree
> whether we want to remain bug-compatible with the old
> least-common-multiple-of-the-periods behavior.  I would vote for not,
> but it's certainly a debatable thing.

+1 on removing LCM.

The behavior of multiple targetlist SRF is so bizarre that it's
incredible to believe anyone would reasonably expect it to work that
way. Agree also that casual sane usage of target list SRF is
incredibly common via generate_series() and unnest() etc is
exceptionally common...better not to break those cases without a
better justification than code simplicity.

merlin

  10. Re: Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-05-23T18:30:13Z

    On Mon, May 23, 2016 at 1:44 PM, David Fetter <david@fetter.org> wrote:

> On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote:
> > David Fetter <david@fetter.org> writes:
> > > On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote:
> > >> This seems a bridge too far to me.  It's just way too common to do
> > >> "select generate_series(1,n)".  We could tell people they have to
> > >> rewrite to "select * from generate_series(1,n)", but it would be far
> > >> more polite to do that for them.
> >
> > > How about making "TABLE generate_series(1,n)" work?  It's even
> > > shorter in exchange for some cognitive load.
> >
> > No thanks --- the word after TABLE ought to be a table name, not some
> > arbitrary expression.  That's way too much mess to save one keystroke.
>
> It's not just about saving a keystroke.  This change would go with
> removing the ability to do SRFs in the target list of a SELECT
> query.
>

​If you want to make an argument for doing this regardless of the target
list SRF change by all means - but it does absolutely nothing to mitigate
the breakage that would result if we choose this path.

David J.​

  11. Re: Changed SRF in targetlist handling

    David Fetter <david@fetter.org> — 2016-05-23T19:13:44Z

    On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
> On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> discussing executor performance with a number of people at pgcon,
> >> several hackers - me included - complained about the additional
> >> complexity, both code and runtime, required to handle SRFs in the target
> >> list.
> >
> > Yeah, this has been an annoyance for a long time.
> >
> >> One idea I circulated was to fix that by interjecting a special executor
> >> node to process SRF containing targetlists (reusing Result possibly?).
> >> That'd allow to remove the isDone argument from ExecEval*/ExecProject*
> >> and get rid of ps_TupFromTlist which is fairly ugly.
> >
> > Would that not lead to, in effect, duplicating all of execQual.c?  The new
> > executor node would still have to be prepared to process all expression
> > node types.
> >
> >> Robert suggested - IIRC mentioning previous on-list discussion - to
> >> instead rewrite targetlist SRFs into lateral joins. My gut feeling is
> >> that that'd be a larger undertaking, with significant semantics changes.
> >
> > Yes, this was discussed on-list awhile back (I see David found a reference
> > already).  I think it's feasible, although we'd first have to agree
> > whether we want to remain bug-compatible with the old
> > least-common-multiple-of-the-periods behavior.  I would vote for not,
> > but it's certainly a debatable thing.
> 
> +1 on removing LCM.

As a green field project, that would make total sense.  As a thing
decades in, it's not clear to me that that would break less stuff or
break it worse than simply disallowing SRFs in the target list, which
has been rejected on bugward-compatibility grounds.  I suspect it
would be even worse because disallowing SRFs in target lists would at
least be obvious and localized when it broke code.

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

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

  12. Re: Changed SRF in targetlist handling

    Merlin Moncure <mmoncure@gmail.com> — 2016-05-23T19:39:54Z

    On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
> On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
>> On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > Andres Freund <andres@anarazel.de> writes:
>> >> discussing executor performance with a number of people at pgcon,
>> >> several hackers - me included - complained about the additional
>> >> complexity, both code and runtime, required to handle SRFs in the target
>> >> list.
>> >
>> > Yeah, this has been an annoyance for a long time.
>> >
>> >> One idea I circulated was to fix that by interjecting a special executor
>> >> node to process SRF containing targetlists (reusing Result possibly?).
>> >> That'd allow to remove the isDone argument from ExecEval*/ExecProject*
>> >> and get rid of ps_TupFromTlist which is fairly ugly.
>> >
>> > Would that not lead to, in effect, duplicating all of execQual.c?  The new
>> > executor node would still have to be prepared to process all expression
>> > node types.
>> >
>> >> Robert suggested - IIRC mentioning previous on-list discussion - to
>> >> instead rewrite targetlist SRFs into lateral joins. My gut feeling is
>> >> that that'd be a larger undertaking, with significant semantics changes.
>> >
>> > Yes, this was discussed on-list awhile back (I see David found a reference
>> > already).  I think it's feasible, although we'd first have to agree
>> > whether we want to remain bug-compatible with the old
>> > least-common-multiple-of-the-periods behavior.  I would vote for not,
>> > but it's certainly a debatable thing.
>>
>> +1 on removing LCM.
>
> As a green field project, that would make total sense.  As a thing
> decades in, it's not clear to me that that would break less stuff or
> break it worse than simply disallowing SRFs in the target list, which
> has been rejected on bugward-compatibility grounds.  I suspect it
> would be even worse because disallowing SRFs in target lists would at
> least be obvious and localized when it broke code.

If I'm reading this correctly, it sounds to me like you are making the
case that removing target list SRF completely would somehow cause less
breakage than say, rewriting it to a LATERAL based implementation for
example.  With more than a little forbearance, let's just say I don't
agree.

merlin

  13. Re: Changed SRF in targetlist handling

    Joe Conway <mail@joeconway.com> — 2016-05-23T19:58:20Z

    On 05/23/2016 12:39 PM, Merlin Moncure wrote:
> On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
>> On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
>>> On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Andres Freund <andres@anarazel.de> writes:
>>>>> discussing executor performance with a number of people at pgcon,
>>>>> several hackers - me included - complained about the additional
>>>>> complexity, both code and runtime, required to handle SRFs in the target
>>>>> list.
>>>>
>>>> Yeah, this has been an annoyance for a long time.
>>>>
>>>>> One idea I circulated was to fix that by interjecting a special executor
>>>>> node to process SRF containing targetlists (reusing Result possibly?).
>>>>> That'd allow to remove the isDone argument from ExecEval*/ExecProject*
>>>>> and get rid of ps_TupFromTlist which is fairly ugly.
>>>>
>>>> Would that not lead to, in effect, duplicating all of execQual.c?  The new
>>>> executor node would still have to be prepared to process all expression
>>>> node types.
>>>>
>>>>> Robert suggested - IIRC mentioning previous on-list discussion - to
>>>>> instead rewrite targetlist SRFs into lateral joins. My gut feeling is
>>>>> that that'd be a larger undertaking, with significant semantics changes.
>>>>
>>>> Yes, this was discussed on-list awhile back (I see David found a reference
>>>> already).  I think it's feasible, although we'd first have to agree
>>>> whether we want to remain bug-compatible with the old
>>>> least-common-multiple-of-the-periods behavior.  I would vote for not,
>>>> but it's certainly a debatable thing.
>>>
>>> +1 on removing LCM.
>>
>> As a green field project, that would make total sense.  As a thing
>> decades in, it's not clear to me that that would break less stuff or
>> break it worse than simply disallowing SRFs in the target list, which
>> has been rejected on bugward-compatibility grounds.  I suspect it
>> would be even worse because disallowing SRFs in target lists would at
>> least be obvious and localized when it broke code.
> 
> If I'm reading this correctly, it sounds to me like you are making the
> case that removing target list SRF completely would somehow cause less
> breakage than say, rewriting it to a LATERAL based implementation for
> example.  With more than a little forbearance, let's just say I don't
> agree.

I'm not necessarily saying that we should totally remove target list
SRFs, but I will point out it has been deprecated ever since SRFs were
first introduced:

http://www.postgresql.org/docs/7.3/static/xfunc-sql.html

  "Currently, functions returning sets may also be called in the target
   list of a SELECT query. For each row that the SELECT generates by
   itself, the function returning set is invoked, and an output row is
   generated for each element of the function's result set. Note,
   however, that this capability is deprecated and may be removed in
   future releases."

I would be in favor of rewriting it to a LATERAL, but that would not be
backwards compatible entirely either IIUC.

I'll also note that, unless I missed something, we also have to consider
that the capability to pipeline results is still only available in the
target list.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  14. Re: Changed SRF in targetlist handling

    David Fetter <david@fetter.org> — 2016-05-23T20:05:03Z

    On Mon, May 23, 2016 at 02:39:54PM -0500, Merlin Moncure wrote:
> On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
> > On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
> >> On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> > Andres Freund <andres@anarazel.de> writes:
> >> >> discussing executor performance with a number of people at pgcon,
> >> >> several hackers - me included - complained about the additional
> >> >> complexity, both code and runtime, required to handle SRFs in the target
> >> >> list.
> >> >
> >> > Yeah, this has been an annoyance for a long time.
> >> >
> >> >> One idea I circulated was to fix that by interjecting a special executor
> >> >> node to process SRF containing targetlists (reusing Result possibly?).
> >> >> That'd allow to remove the isDone argument from ExecEval*/ExecProject*
> >> >> and get rid of ps_TupFromTlist which is fairly ugly.
> >> >
> >> > Would that not lead to, in effect, duplicating all of execQual.c?  The new
> >> > executor node would still have to be prepared to process all expression
> >> > node types.
> >> >
> >> >> Robert suggested - IIRC mentioning previous on-list discussion - to
> >> >> instead rewrite targetlist SRFs into lateral joins. My gut feeling is
> >> >> that that'd be a larger undertaking, with significant semantics changes.
> >> >
> >> > Yes, this was discussed on-list awhile back (I see David found a reference
> >> > already).  I think it's feasible, although we'd first have to agree
> >> > whether we want to remain bug-compatible with the old
> >> > least-common-multiple-of-the-periods behavior.  I would vote for not,
> >> > but it's certainly a debatable thing.
> >>
> >> +1 on removing LCM.
> >
> > As a green field project, that would make total sense.  As a thing
> > decades in, it's not clear to me that that would break less stuff or
> > break it worse than simply disallowing SRFs in the target list, which
> > has been rejected on bugward-compatibility grounds.  I suspect it
> > would be even worse because disallowing SRFs in target lists would at
> > least be obvious and localized when it broke code.
> 
> If I'm reading this correctly, it sounds to me like you are making the
> case that removing target list SRF completely would somehow cause less
> breakage than say, rewriting it to a LATERAL based implementation for
> example.

Yes.

Making SRFs in target lists throw an error is a thing that will be
pretty straightforward to deal with in extant code bases, whatever
size of pain in the neck it might be.  The line of code that caused
the error would be very clear, and the fix would be very obvious.

Making their behavior different in some way that throws no warnings is
guaranteed to cause subtle and hard to track bugs in extant code
bases.  We lost not a few existing users when we caused similar
knock-ons in 8.3 by removing automated casts to text.

I am no longer advocating for removing the functionality.  I am just
pointing out that the knock-on effects of changing the functionality
may well cause more pain than the ones from removing it entirely.

> With more than a little forbearance, let's just say I don't agree.

If you'd be so kind as to explain your reasons, I think we'd all
benefit.

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

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

  15. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-23T20:15:13Z

    Merlin Moncure <mmoncure@gmail.com> writes:
> On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
>> On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
>>> +1 on removing LCM.

>> As a green field project, that would make total sense.  As a thing
>> decades in, it's not clear to me that that would break less stuff or
>> break it worse than simply disallowing SRFs in the target list, which
>> has been rejected on bugward-compatibility grounds.  I suspect it
>> would be even worse because disallowing SRFs in target lists would at
>> least be obvious and localized when it broke code.

> If I'm reading this correctly, it sounds to me like you are making the
> case that removing target list SRF completely would somehow cause less
> breakage than say, rewriting it to a LATERAL based implementation for
> example.  With more than a little forbearance, let's just say I don't
> agree.

We should consider single and multiple SRFs in a targetlist as distinct
use-cases; only the latter has got weird properties.

There are several things we could potentially do with multiple SRFs in
the same targetlist.  In increasing order of backwards compatibility and
effort required:

1. Throw error if there's more than one SRF.

2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...).  This would
have the same behavior as before if the SRFs all return the same number
of rows, and otherwise would behave differently.

3. Rewrite into some other construct that still ends up as a FunctionScan
RTE node, but has the old LCM behavior if the SRFs produce different
numbers of rows.  (Perhaps we would not need to expose this construct
as something directly SQL-visible.)

It's certainly arguable that the common use-cases for SRF-in-tlist
don't have more than one SRF per tlist, and thus that implementing #1
would be an appropriate amount of effort.  It's worth noting also that
the LCM behavior has been repeatedly reported as a bug, and therefore
that if we do #3 we'll be expending very substantial effort to be
literally bug-compatible with ancient behavior that no one in the
current development group thinks is well-designed.  As far as #2 goes,
it would have the advantage that code depending on the same-number-of-
rows case would continue to work as before.  David has a point that it
would silently break application code that's actually depending on the
LCM behavior, but how much of that is there likely to be, really?

[ reflects a bit... ]  I guess there is room for an option 2-and-a-half:

2.5. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...), but decorate
the FunctionScan RTE to tell the executor to throw an error if the SRFs
don't all return the same number of rows, rather than silently
null-padding.  This would have the same behavior as before for the sane
case, and would be very not-silent about cases where apps actually invoked
the LCM behavior.  Again, we wouldn't necessarily have to expose such an
option at the SQL level.  (Though it strikes me that such a restriction
could have value in its own right, analogous to the STRICT options that
we've invented in some other places to allow insisting on the expected
numbers of rows being returned.  ROWS FROM STRICT (srf1(), srf2(), ...),
anybody?)

			regards, tom lane

  16. Re: Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-05-23T20:17:15Z

    On Mon, May 23, 2016 at 4:05 PM, David Fetter <david@fetter.org> wrote:

> On Mon, May 23, 2016 at 02:39:54PM -0500, Merlin Moncure wrote:
> > On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
> > > On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
> > >> On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> > Andres Freund <andres@anarazel.de> writes:
> > >> >> discussing executor performance with a number of people at pgcon,
> > >> >> several hackers - me included - complained about the additional
> > >> >> complexity, both code and runtime, required to handle SRFs in the
> target
> > >> >> list.
> > >> >
> > >> > Yeah, this has been an annoyance for a long time.
> > >> >
> > >> >> One idea I circulated was to fix that by interjecting a special
> executor
> > >> >> node to process SRF containing targetlists (reusing Result
> possibly?).
> > >> >> That'd allow to remove the isDone argument from
> ExecEval*/ExecProject*
> > >> >> and get rid of ps_TupFromTlist which is fairly ugly.
> > >> >
> > >> > Would that not lead to, in effect, duplicating all of execQual.c?
> The new
> > >> > executor node would still have to be prepared to process all
> expression
> > >> > node types.
> > >> >
> > >> >> Robert suggested - IIRC mentioning previous on-list discussion - to
> > >> >> instead rewrite targetlist SRFs into lateral joins. My gut feeling
> is
> > >> >> that that'd be a larger undertaking, with significant semantics
> changes.
> > >> >
> > >> > Yes, this was discussed on-list awhile back (I see David found a
> reference
> > >> > already).  I think it's feasible, although we'd first have to agree
> > >> > whether we want to remain bug-compatible with the old
> > >> > least-common-multiple-of-the-periods behavior.  I would vote for
> not,
> > >> > but it's certainly a debatable thing.
> > >>
> > >> +1 on removing LCM.
> > >
> > > As a green field project, that would make total sense.  As a thing
> > > decades in, it's not clear to me that that would break less stuff or
> > > break it worse than simply disallowing SRFs in the target list, which
> > > has been rejected on bugward-compatibility grounds.  I suspect it
> > > would be even worse because disallowing SRFs in target lists would at
> > > least be obvious and localized when it broke code.
> >
> > If I'm reading this correctly, it sounds to me like you are making the
> > case that removing target list SRF completely would somehow cause less
> > breakage than say, rewriting it to a LATERAL based implementation for
> > example.
>
> Yes.
>
> Making SRFs in target lists throw an error is a thing that will be
> pretty straightforward to deal with in extant code bases, whatever
> size of pain in the neck it might be.  The line of code that caused
> the error would be very clear, and the fix would be very obvious.
>
> Making their behavior different in some way that throws no warnings is
> guaranteed to cause subtle and hard to track bugs in extant code
> bases.


​I'm advocating that if a presently allowed SRF-in-target-list is allowed
to remain it executes using the same semantics it has today.  In all other
cases, including LCM, if the present behavior is undesirable to maintain we
throw an error.  I'd hope that such an error can be written in such a way
as to name the offending function or functions.

If the user of a complex query doesn't want to expend the effort to locate
the specific instance of SRF that is in violation they will still have the
option to rewrite all of their uses in that particular query.

David J.

  17. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-23T20:17:55Z

    Joe Conway <mail@joeconway.com> writes:
> I would be in favor of rewriting it to a LATERAL, but that would not be
> backwards compatible entirely either IIUC.

It could be made so, I think, but it may be more trouble than it's worth;
see my previous message.

> I'll also note that, unless I missed something, we also have to consider
> that the capability to pipeline results is still only available in the
> target list.

Yes, we would definitely want to improve nodeFunctionscan.c to perform
better for ValuePerCall SRFs.  But that has value independently of this.

			regards, tom lane

  18. Re: Changed SRF in targetlist handling

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-05-23T20:24:45Z

    Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:

> > I'll also note that, unless I missed something, we also have to consider
> > that the capability to pipeline results is still only available in the
> > target list.
> 
> Yes, we would definitely want to improve nodeFunctionscan.c to perform
> better for ValuePerCall SRFs.  But that has value independently of this.

Ah, so that's what "pipeline results" mean!  I hadn't gotten that.  I
agree; Abhijit had a patch or a plan for this, a long time ago ...

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  19. Re: Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-05-23T20:32:07Z

    On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Merlin Moncure <mmoncure@gmail.com> writes:
> > On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
> >> On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
> >>> +1 on removing LCM.
>
> >> As a green field project, that would make total sense.  As a thing
> >> decades in, it's not clear to me that that would break less stuff or
> >> break it worse than simply disallowing SRFs in the target list, which
> >> has been rejected on bugward-compatibility grounds.  I suspect it
> >> would be even worse because disallowing SRFs in target lists would at
> >> least be obvious and localized when it broke code.
>
> > If I'm reading this correctly, it sounds to me like you are making the
> > case that removing target list SRF completely would somehow cause less
> > breakage than say, rewriting it to a LATERAL based implementation for
> > example.  With more than a little forbearance, let's just say I don't
> > agree.
>
> We should consider single and multiple SRFs in a targetlist as distinct
> use-cases; only the latter has got weird properties.
>
> There are several things we could potentially do with multiple SRFs in
> the same targetlist.  In increasing order of backwards compatibility and
> effort required:
>
> 1. Throw error if there's more than one SRF.
>
> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...).  This would
> have the same behavior as before if the SRFs all return the same number
> of rows, and otherwise would behave differently.
>
> 3. Rewrite into some other construct that still ends up as a FunctionScan
> RTE node, but has the old LCM behavior if the SRFs produce different
> numbers of rows.  (Perhaps we would not need to expose this construct
> as something directly SQL-visible.)
>
> It's certainly arguable that the common use-cases for SRF-in-tlist
> don't have more than one SRF per tlist, and thus that implementing #1
> would be an appropriate amount of effort.  It's worth noting also that
> the LCM behavior has been repeatedly reported as a bug, and therefore
> that if we do #3 we'll be expending very substantial effort to be
> literally bug-compatible with ancient behavior that no one in the
> current development group thinks is well-designed.  As far as #2 goes,
> it would have the advantage that code depending on the same-number-of-
> rows case would continue to work as before.  David has a point that it
> would silently break application code that's actually depending on the
> LCM behavior, but how much of that is there likely to be, really?
>
> [ reflects a bit... ]  I guess there is room for an option 2-and-a-half:
>
> 2.5. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...), but decorate
> the FunctionScan RTE to tell the executor to throw an error if the SRFs
> don't all return the same number of rows, rather than silently
> null-padding.  This would have the same behavior as before for the sane
> case, and would be very not-silent about cases where apps actually invoked
> the LCM behavior.  Again, we wouldn't necessarily have to expose such an
> option at the SQL level.  (Though it strikes me that such a restriction
> could have value in its own right, analogous to the STRICT options that
> we've invented in some other places to allow insisting on the expected
> numbers of rows being returned.  ROWS FROM STRICT (srf1(), srf2(), ...),
> anybody?)
>

​I'd let the engineers decide between 1, 2.5, and 3 - but if we accomplish
our goals while implementing #3 I'd say that would be the best outcome for
the community as whole.

We don't have the luxury of providing a safe-usage mode where people
writing new queries get the error but pre-existing queries are considered
OK.  We will have to rely upon education and deal with the occasional bug
report but our long-time customers, even if only a minority would be
affected, will appreciate the effort taken to not break code that has been
working for a long time.

The minority is likely small enough to at least make options 1 and 2.5
viable though I'd think we make an effort to avoid #1.

​David J.​

  20. Re: Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-05-23T20:33:41Z

    On Mon, May 23, 2016 at 4:24 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

> Tom Lane wrote:
> > Joe Conway <mail@joeconway.com> writes:
>
> > > I'll also note that, unless I missed something, we also have to
> consider
> > > that the capability to pipeline results is still only available in the
> > > target list.
> >
> > Yes, we would definitely want to improve nodeFunctionscan.c to perform
> > better for ValuePerCall SRFs.  But that has value independently of this.
>
> Ah, so that's what "pipeline results" mean!  I hadn't gotten that.  I
> agree; Abhijit had a patch or a plan for this, a long time ago ...
>
>
​Is this sidebar strictly an implementation detail, not user visible?

David J.

  21. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-23T20:42:46Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Mon, May 23, 2016 at 4:24 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
>> Ah, so that's what "pipeline results" mean!  I hadn't gotten that.  I
>> agree; Abhijit had a patch or a plan for this, a long time ago ...

> Is this sidebar strictly an implementation detail, not user visible?

Hmm.  It could be visible in the sense that the execution of multiple
functions in one ROWS FROM() construct could be interleaved, while
(I think) the current implementation runs each one to completion
serially.  But if you're writing code that assumes that, I think you
should not be very surprised when we break it.  In any case, that
would not affect the proposed translation for SRFs-in-tlist, since
those have that behavior today.

			regards, tom lane

  22. Re: Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-05-23T21:37:06Z

    On Mon, May 23, 2016 at 4:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> "David G. Johnston" <david.g.johnston@gmail.com> writes:
> > On Mon, May 23, 2016 at 4:24 PM, Alvaro Herrera <
> alvherre@2ndquadrant.com>
> > wrote:
> >> Ah, so that's what "pipeline results" mean!  I hadn't gotten that.  I
> >> agree; Abhijit had a patch or a plan for this, a long time ago ...
>
> > ​Is this sidebar strictly an implementation detail, not user visible?
>
> Hmm.  It could be visible in the sense that the execution of multiple
> functions in one ROWS FROM() construct could be interleaved, while
> (I think) the current implementation runs each one to completion
> serially.  But if you're writing code that assumes that, I think you
> should not be very surprised when we break it.  In any case, that
> would not affect the proposed translation for SRFs-in-tlist, since
> those have that behavior today.
>

Thanks

​Sounds like "zipper results" would be a better term for it...but, yes, if
that's the general context it falls into implementation from my
perspective.​

​But then I don't get Joe's point - if its an implementation detail why
should it matter if rewriting the SRF-in-tlist to be laterals changes
execution from a serial to an interleaved​ implementation.  Plus, Joe's
claim: "the capability to pipeline results is still only available in the
target list", and yours above are at odds since you claim the rewritten
behavior is the same today.  Is there a disconnect in knowledge or are you
talking about different things?

​David J.​

  23. Re: Changed SRF in targetlist handling

    Joe Conway <mail@joeconway.com> — 2016-05-23T21:58:51Z

    On 05/23/2016 02:37 PM, David G. Johnston wrote:
> ​But then I don't get Joe's point - if its an implementation detail why
> should it matter if rewriting the SRF-in-tlist to be laterals changes
> execution from a serial to an interleaved​ implementation.  Plus, Joe's
> claim: "the capability to pipeline results is still only available in
> the target list", and yours above are at odds since you claim the
> rewritten behavior is the same today.  Is there a disconnect in
> knowledge or are you talking about different things?

Unless there have been recent changes which I missed, ValuePerCall SRFs
are still run to completion in one go, when executed in the FROM clause,
but they project one-row-at-a-time in the target list. If your SRF
returns many-many rows, the problem with the former case is that the
entire thing has to be materialized in memory.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

  24. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-05-25T18:57:12Z

    On 2016-05-23 13:10:29 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > One idea I circulated was to fix that by interjecting a special executor
> > node to process SRF containing targetlists (reusing Result possibly?).
> > That'd allow to remove the isDone argument from ExecEval*/ExecProject*
> > and get rid of ps_TupFromTlist which is fairly ugly.
> 
> Would that not lead to, in effect, duplicating all of execQual.c?  The new
> executor node would still have to be prepared to process all expression
> node types.

I don't think it necessarily has to. ISTM that if we add a version of
ExecProject()/ExecTargetList() that continues returning multiple rows,
we can make the knowledge about the one type of expression we allow to
return multiple rows.  That'd require a bit of uglyness to implement
stuff like
SELECT generate_series(1, 2)::text, generate_series(1, 2) * 5;
etc. It seems we'd basically have to do one projection step for the
SRFs, and then another for the rest.  I'm inclined to think that's
acceptable to get rid of a lot of the related uglyness.


> > One issue with removing targetlist SRFs is that they're currently
> > considerably faster than SRFs in FROM:
> 
> I suspect that depends greatly on your test case.  But in any case
> we could put more effort into optimizing nodeFunctionscan.

I doubt you'll find cases where it's significantly the other way round
for percall SRFs. The fundamental issue is that targetlist SRFs don't
have to spill to a tuplestore, whereas nodeFunctionscan ones have to
(even if they're percall).

  25. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-25T19:02:23Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-05-23 13:10:29 -0400, Tom Lane wrote:
>> Would that not lead to, in effect, duplicating all of execQual.c?  The new
>> executor node would still have to be prepared to process all expression
>> node types.

> I don't think it necessarily has to. ISTM that if we add a version of
> ExecProject()/ExecTargetList() that continues returning multiple rows,
> we can make the knowledge about the one type of expression we allow to
> return multiple rows.  That'd require a bit of uglyness to implement
> stuff like
> SELECT generate_series(1, 2)::text, generate_series(1, 2) * 5;
> etc. It seems we'd basically have to do one projection step for the
> SRFs, and then another for the rest.  I'm inclined to think that's
> acceptable to get rid of a lot of the related uglyness.

[ shrug... ]  That seems like it's morally equivalent to (but uglier than)
what I wanted to do, which is to teach the planner to rewrite the query to
put the SRFs into a lateral FROM item.  Splitting the tlist into two
levels will work out to be exactly the same rewriting problem.

			regards, tom lane

  26. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-05-25T19:14:25Z

    On 2016-05-25 15:02:23 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-05-23 13:10:29 -0400, Tom Lane wrote:
> >> Would that not lead to, in effect, duplicating all of execQual.c?  The new
> >> executor node would still have to be prepared to process all expression
> >> node types.
> 
> > I don't think it necessarily has to. ISTM that if we add a version of
> > ExecProject()/ExecTargetList() that continues returning multiple rows,
> > we can make the knowledge about the one type of expression we allow to
> > return multiple rows.  That'd require a bit of uglyness to implement
> > stuff like
> > SELECT generate_series(1, 2)::text, generate_series(1, 2) * 5;
> > etc. It seems we'd basically have to do one projection step for the
> > SRFs, and then another for the rest.  I'm inclined to think that's
> > acceptable to get rid of a lot of the related uglyness.
> 
> [ shrug... ]  That seems like it's morally equivalent to (but uglier than)
> what I wanted to do, which is to teach the planner to rewrite the query to
> put the SRFs into a lateral FROM item.  Splitting the tlist into two
> levels will work out to be exactly the same rewriting problem.

I think that depends on how bug compatible we want to be. It seems
harder to get the (rather odd!) lockstep iteration behaviour between two
SRFS with the LATERAL approach?

tpch[6098][1]=# SELECT generate_series(1, 3), generate_series(1,3);
┌─────────────────┬─────────────────┐
│ generate_series │ generate_series │
├─────────────────┼─────────────────┤
│               1 │               1 │
│               2 │               2 │
│               3 │               3 │
└─────────────────┴─────────────────┘
(3 rows)

tpch[6098][1]=# SELECT generate_series(1, 3), generate_series(1,4);
┌─────────────────┬─────────────────┐
│ generate_series │ generate_series │
├─────────────────┼─────────────────┤
│               1 │               1 │
│               2 │               2 │
│               3 │               3 │
│               1 │               4 │
│               2 │               1 │
│               3 │               2 │
│               1 │               3 │
│               2 │               4 │
│               3 │               1 │
│               1 │               2 │
│               2 │               3 │
│               3 │               4 │
└─────────────────┴─────────────────┘
(12 rows)


Regards,

Andres

  27. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-25T19:20:03Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-05-25 15:02:23 -0400, Tom Lane wrote:
>> [ shrug... ]  That seems like it's morally equivalent to (but uglier than)
>> what I wanted to do, which is to teach the planner to rewrite the query to
>> put the SRFs into a lateral FROM item.  Splitting the tlist into two
>> levels will work out to be exactly the same rewriting problem.

> I think that depends on how bug compatible we want to be. It seems
> harder to get the (rather odd!) lockstep iteration behaviour between two
> SRFS with the LATERAL approach?

We could certainly make a variant behavior in nodeFunctionscan.c that
emulates that, if we feel that being exactly bug-compatible on the point
is actually what we want.  I'm dubious about that though, not least
because I don't think *anyone* actually believes that that behavior isn't
broken.  Did you read my upthread message suggesting assorted compromise
choices?

			regards, tom lane

  28. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-05-25T20:32:57Z

    On 2016-05-25 15:20:03 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-05-25 15:02:23 -0400, Tom Lane wrote:
> >> [ shrug... ]  That seems like it's morally equivalent to (but uglier than)
> >> what I wanted to do, which is to teach the planner to rewrite the query to
> >> put the SRFs into a lateral FROM item.  Splitting the tlist into two
> >> levels will work out to be exactly the same rewriting problem.
> 
> > I think that depends on how bug compatible we want to be. It seems
> > harder to get the (rather odd!) lockstep iteration behaviour between two
> > SRFS with the LATERAL approach?
> 
> We could certainly make a variant behavior in nodeFunctionscan.c that
> emulates that, if we feel that being exactly bug-compatible on the point
> is actually what we want.  I'm dubious about that though, not least
> because I don't think *anyone* actually believes that that behavior isn't
> broken.  Did you read my upthread message suggesting assorted compromise
> choices?

You mean https://www.postgresql.org/message-id/21076.1464034513@sss.pgh.pa.us ?
If so, yes.

If we go with rewriting this into LATERAL, I'd vote for 2.5 (trailed by
option 1), that'd keep most of the functionality, and would break
visibly rather than invisibly in the cases where not.

I guess you're not planning to work on this?

Greetings,

Andres Freund

  29. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-25T20:55:23Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-05-25 15:20:03 -0400, Tom Lane wrote:
>> We could certainly make a variant behavior in nodeFunctionscan.c that
>> emulates that, if we feel that being exactly bug-compatible on the point
>> is actually what we want.  I'm dubious about that though, not least
>> because I don't think *anyone* actually believes that that behavior isn't
>> broken.  Did you read my upthread message suggesting assorted compromise
>> choices?

> You mean https://www.postgresql.org/message-id/21076.1464034513@sss.pgh.pa.us ?
> If so, yes.

> If we go with rewriting this into LATERAL, I'd vote for 2.5 (trailed by
> option 1), that'd keep most of the functionality, and would break
> visibly rather than invisibly in the cases where not.

2.5 would be okay with me.

> I guess you're not planning to work on this?

Well, not right now, as it's clearly too late for 9.6.  I might hack on
it later if nobody beats me to it.

			regards, tom lane

  30. Re: Changed SRF in targetlist handling

    Merlin Moncure <mmoncure@gmail.com> — 2016-06-03T18:22:56Z

    On Wed, May 25, 2016 at 3:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2016-05-25 15:20:03 -0400, Tom Lane wrote:
>>> We could certainly make a variant behavior in nodeFunctionscan.c that
>>> emulates that, if we feel that being exactly bug-compatible on the point
>>> is actually what we want.  I'm dubious about that though, not least
>>> because I don't think *anyone* actually believes that that behavior isn't
>>> broken.  Did you read my upthread message suggesting assorted compromise
>>> choices?
>
>> You mean https://www.postgresql.org/message-id/21076.1464034513@sss.pgh.pa.us ?
>> If so, yes.
>
>> If we go with rewriting this into LATERAL, I'd vote for 2.5 (trailed by
>> option 1), that'd keep most of the functionality, and would break
>> visibly rather than invisibly in the cases where not.
>
> 2.5 would be okay with me.
>
>> I guess you're not planning to work on this?
>
> Well, not right now, as it's clearly too late for 9.6.  I might hack on
> it later if nobody beats me to it.

Curious if this approach will also rewrite:
select generate_series(1,generate_series(1,3)) s;

...into
select s from generate_series(1,3) x cross join lateral generate_series(1,x) s;

another interesting case today is:
create sequence s;
select generate_series(1,nextval('s')), generate_series(1,nextval('s'));

this statement never terminates.  a lateral rewrite of this query
would always terminate with much better defined and well understood
behaviors -- this is good.

merlin

  31. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-06-03T19:52:12Z

    Merlin Moncure <mmoncure@gmail.com> writes:
> On Wed, May 25, 2016 at 3:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> If we go with rewriting this into LATERAL, I'd vote for 2.5 (trailed by
>>> option 1), that'd keep most of the functionality, and would break
>>> visibly rather than invisibly in the cases where not.
>> 2.5 would be okay with me.

> Curious if this approach will also rewrite:
> select generate_series(1,generate_series(1,3)) s;
> ...into
> select s from generate_series(1,3) x cross join lateral generate_series(1,x) s;

Yeah, that would be the idea.

> another interesting case today is:
> create sequence s;
> select generate_series(1,nextval('s')), generate_series(1,nextval('s'));

> this statement never terminates.  a lateral rewrite of this query
> would always terminate with much better defined and well understood
> behaviors -- this is good.

Interesting example demonstrating that 100% bug compatibility is not
possible.  But as you say, most people would probably prefer the other
behavior anyhow.

			regards, tom lane

  32. Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-06-03T20:13:37Z

    On Friday, June 3, 2016, Tom Lane <tgl@sss.pgh.pa.us
<javascript:_e(%7B%7D,'cvml','tgl@sss.pgh.pa.us');>> wrote:

> Merlin Moncure <mmoncure@gmail.com> writes:
> > On Wed, May 25, 2016 at 3:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> >>> If we go with rewriting this into LATERAL, I'd vote for 2.5 (trailed by
> >>> option 1), that'd keep most of the functionality, and would break
> >>> visibly rather than invisibly in the cases where not.
> >> 2.5 would be okay with me.
>
> > Curious if this approach will also rewrite:
> > select generate_series(1,generate_series(1,3)) s;
> > ...into
> > select s from generate_series(1,3) x cross join lateral
> generate_series(1,x) s;
>
> Yeah, that would be the idea.
>
>
Ok...  It's only a single srf as far as the outer query is concerned so
while it is odd the behavior is well defined and can be transformed while
giving the same result.


> > another interesting case today is:
> > create sequence s;
> > select generate_series(1,nextval('s')), generate_series(1,nextval('s'));
>
> > this statement never terminates.  a lateral rewrite of this query
> > would always terminate with much better defined and well understood
> > behaviors -- this is good.
>
> Interesting example demonstrating that 100% bug compatibility is not
> possible.  But as you say, most people would probably prefer the other
> behavior anyhow.
>
>
If taking the 2.5 approach this one would fail as opposed to being
rewritten.

This could be an exception to the policy in #3 and would be ok in #2.  It
would fail in #1.

Given the apparent general consensus for 2.5 and the lack of working field
versions of this form the error seems like a no brainer.

David J.

  33. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-06-03T20:38:29Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> On Friday, June 3, 2016, Tom Lane <tgl@sss.pgh.pa.us
> <javascript:_e(%7B%7D,'cvml','tgl@sss.pgh.pa.us');>> wrote:
>> Merlin Moncure <mmoncure@gmail.com> writes:
>>> another interesting case today is:
>>> create sequence s;
>>> select generate_series(1,nextval('s')), generate_series(1,nextval('s'));

> If taking the 2.5 approach this one would fail as opposed to being
> rewritten.

Well, it'd be rewritten and then would fail at runtime because of the SRF
calls not producing the same number of rows.  But even option #3 would not
be strictly bug-compatible because it would (I imagine) evaluate the
arguments of each SRF only once.  The reason this case doesn't terminate
in the current implementation is that it re-evaluates the SRF arguments
each time we start a SRF over.  That's just weird ...

			regards, tom lane

  34. Re: Changed SRF in targetlist handling

    Robert Haas <robertmhaas@gmail.com> — 2016-06-06T15:30:54Z

    On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> We should consider single and multiple SRFs in a targetlist as distinct
> use-cases; only the latter has got weird properties.
>
> There are several things we could potentially do with multiple SRFs in
> the same targetlist.  In increasing order of backwards compatibility and
> effort required:
>
> 1. Throw error if there's more than one SRF.
>
> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...).  This would
> have the same behavior as before if the SRFs all return the same number
> of rows, and otherwise would behave differently.

I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()),
LATERAL ROWS FROM (srf2()), ...

The rewrite you propose here seems to NULL-pad rows after the first
SRF is exhausted:

rhaas=# select * from dual, lateral rows from (generate_series(1,3),
generate_series(1,4));
   x   | generate_series | generate_series
-------+-----------------+-----------------
 dummy |               1 |               1
 dummy |               2 |               2
 dummy |               3 |               3
 dummy |                 |               4
(4 rows)

...whereas with a separate LATERAL clause for each row you get this:

rhaas=# select * from dual, lateral rows from (generate_series(1,3))
a, lateral rows from (generate_series(1,4)) b;
   x   | a | b
-------+---+---
 dummy | 1 | 1
 dummy | 1 | 2
 dummy | 1 | 3
 dummy | 1 | 4
 dummy | 2 | 1
 dummy | 2 | 2
 dummy | 2 | 3
 dummy | 2 | 4
 dummy | 3 | 1
 dummy | 3 | 2
 dummy | 3 | 3
 dummy | 3 | 4
(12 rows)

The latter is how I'd expect SRF-in-targetlist to work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  35. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-06-06T15:50:43Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...).  This would
>> have the same behavior as before if the SRFs all return the same number
>> of rows, and otherwise would behave differently.

> I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()),
> LATERAL ROWS FROM (srf2()), ...

No, because then you get the cross-product of multiple SRFs, not the
run-in-lockstep behavior.

> The rewrite you propose here seems to NULL-pad rows after the first
> SRF is exhausted:

Yes.  That's why I said it's not compatible if the SRFs don't all return
the same number of rows.  It seems like a reasonable definition to me
though, certainly much more reasonable than the current run-until-LCM
behavior.

> The latter is how I'd expect SRF-in-targetlist to work.

That's not even close to how it works now.  It would break *every*
existing application that has multiple SRFs in the tlist, not just
the ones whose SRFs return different numbers of rows.  And I'm not
convinced that it's a more useful behavior.

			regards, tom lane

  36. Re: Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-06-06T16:30:31Z

    On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...).  This would
> >> have the same behavior as before if the SRFs all return the same number
> >> of rows, and otherwise would behave differently.
>
> > I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()),
> > LATERAL ROWS FROM (srf2()), ...
>
> No, because then you get the cross-product of multiple SRFs, not the
> run-in-lockstep behavior.
>
> > The rewrite you propose here seems to NULL-pad rows after the first
> > SRF is exhausted:
>
> Yes.  That's why I said it's not compatible if the SRFs don't all return
> the same number of rows.  It seems like a reasonable definition to me
> though, certainly much more reasonable than the current run-until-LCM
> behavior.
>

​IOW, this is why this mode query has to fail.
​


>
> > The latter is how I'd expect SRF-in-targetlist to work.
>
> That's not even close to how it works now.  It would break *every*
> existing application that has multiple SRFs in the tlist, not just
> the ones whose SRFs return different numbers of rows.  And I'm not
> convinced that it's a more useful behavior.
>

To clarify, the present behavior is basically a combination of both of
Robert's results.

If the SRFs return the same number of rows the first (zippered) result is
returned without an NULL padding.

If the SRFs return a different number of rows the LCM behavior kicks in and
you get Robert's second result.

SELECT generate_series(1, 4), generate_series(1, 4) ORDER BY 1, 2;
is the same as
SELECT * FROM ROWS FROM ( generate_series(1, 4), generate_series(1, 4) );

BUT

​SELECT generate_series(1, 3), generate_series(1, 4) ORDER BY 1, 2;
is the same as
SELECT * FROM ROWS FROM generate_series(1, 3) a, LATERAL ROWS FROM
generate_series(1, 4) b;


Tom's 2.5 proposal basically says we make the former equivalence succeed
and have the later one fail.

The rewrite would be unaware of the cardinality of the SRF and so it cannot
conditionally rewrite the query.  One of the two must be chosen and the
incompatible behavior turned into an error.

David J.

  37. Re: Changed SRF in targetlist handling

    Vik Fearing <vik@2ndquadrant.fr> — 2016-06-06T16:40:05Z

    On 06/06/16 18:30, David G. Johnston wrote:
> To clarify, the present behavior is basically a combination of both of
> Robert's results.
> 
> If the SRFs return the same number of rows the first (zippered) result
> is returned without an NULL padding.
> 
> If the SRFs return a different number of rows the LCM behavior kicks in
> and you get Robert's second result.

No.

> SELECT generate_series(1, 4), generate_series(1, 4) ORDER BY 1, 2;
> is the same as
> SELECT * FROM ROWS FROM ( generate_series(1, 4), generate_series(1, 4) );
> 
> BUT
> 
> ​SELECT generate_series(1, 3), generate_series(1, 4) ORDER BY 1, 2;
> is the same as
> SELECT * FROM ROWS FROM generate_series(1, 3) a, LATERAL ROWS FROM
> generate_series(1, 4) b;

What would you do with:

SELECT generate_series(1, 3), generate_series(1, 6);

?

> Tom's 2.5 proposal basically says we make the former equivalence succeed
> and have the later one fail.
> 
> The rewrite would be unaware of the cardinality of the SRF and so it
> cannot conditionally rewrite the query.  One of the two must be chosen
> and the incompatible behavior turned into an error.


-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

  38. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-06-06T16:41:38Z

    "David G. Johnston" <david.g.johnston@gmail.com> writes:
> If the SRFs return a different number of rows the LCM behavior kicks in and
> you get Robert's second result.

Only if the periods of the SRFs are relatively prime.  That is, neither of
his examples demonstrate the full weirdness of the current behavior; for
that, you need periods that are multiples of each other.  For instance:

SELECT generate_series(1, 2), generate_series(1, 4); 
 generate_series | generate_series 
-----------------+-----------------
               1 |               1
               2 |               2
               1 |               3
               2 |               4
(4 rows)

That doesn't comport with any behavior available from LATERAL.

			regards, tom lane

  39. Re: Changed SRF in targetlist handling

    Robert Haas <robertmhaas@gmail.com> — 2016-06-06T18:15:56Z

    On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...).  This would
>>> have the same behavior as before if the SRFs all return the same number
>>> of rows, and otherwise would behave differently.
>
>> I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()),
>> LATERAL ROWS FROM (srf2()), ...
>
> No, because then you get the cross-product of multiple SRFs, not the
> run-in-lockstep behavior.

Oh.  I assumed that was the expected behavior.  But, ah, what do I know?

>> The rewrite you propose here seems to NULL-pad rows after the first
>> SRF is exhausted:
>
> Yes.  That's why I said it's not compatible if the SRFs don't all return
> the same number of rows.  It seems like a reasonable definition to me
> though, certainly much more reasonable than the current run-until-LCM
> behavior.

I can't argue with that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  40. Re: Changed SRF in targetlist handling

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-06-06T18:31:32Z

    Robert Haas wrote:
> On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...).  This would
> >>> have the same behavior as before if the SRFs all return the same number
> >>> of rows, and otherwise would behave differently.
> >
> >> I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()),
> >> LATERAL ROWS FROM (srf2()), ...
> >
> > No, because then you get the cross-product of multiple SRFs, not the
> > run-in-lockstep behavior.
> 
> Oh.  I assumed that was the expected behavior.  But, ah, what do I know?

Lots, I assume -- but in this case, probably next to nothing, just like
most of us, because what sane person or application would be really
relying on the wacko historical behavior, in order to generate some
collective knowledge?  However, I think that it is possible that
someone, somewhere has two SRFs-in-targetlist that return the same
number of rows and that the current implementation works fine for them;
if we redefine it to work differently, we would break their application
silently, which seems a worse problem than breaking it noisily while
providing an easy way forward (which is to move SRFs to the FROM list)

My vote is to raise an error in the case of more than one SRF in targetlist.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  41. Re: Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-06-06T18:48:27Z

    On Mon, Jun 6, 2016 at 2:31 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

> Robert Haas wrote:
> > On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Robert Haas <robertmhaas@gmail.com> writes:
> > >> On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >>> 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...).  This would
> > >>> have the same behavior as before if the SRFs all return the same
> number
> > >>> of rows, and otherwise would behave differently.
> > >
> > >> I thought the idea was to rewrite it as LATERAL ROWS FROM (srf1()),
> > >> LATERAL ROWS FROM (srf2()), ...
> > >
> > > No, because then you get the cross-product of multiple SRFs, not the
> > > run-in-lockstep behavior.
> >
> > Oh.  I assumed that was the expected behavior.  But, ah, what do I know?
>
> Lots, I assume -- but in this case, probably next to nothing, just like
> most of us, because what sane person or application would be really
> relying on the wacko historical behavior, in order to generate some
> collective knowledge?  However, I think that it is possible that
> someone, somewhere has two SRFs-in-targetlist that return the same
> number of rows and that the current implementation works fine for them;
> if we redefine it to work differently, we would break their application
> silently, which seems a worse problem than breaking it noisily while
> providing an easy way forward (which is to move SRFs to the FROM list)
>
> My vote is to raise an error in the case of more than one SRF in
> targetlist.
>

​As long as someone is willing to put in the effort we can make a subset of
these multiple-SRFs-in-targetlist queries work without any change in the
tabular output, though the processing mechanism might change.​  Your vote
is essentially #1 up-thread which seems the most draconian.  Assuming a
viable option 2.5 or 3 solution is presented would you vote against it
being committed?  If so I'd like to understand why.  I see #1 as basically
OK only if their are technical barriers we cannot overcome - including
performance.

Link to the definition of the various options Tom proposed:

https://www.postgresql.org/message-id/21076.1464034513%40sss.pgh.pa.us

David J.

  42. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-06-06T18:53:41Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Robert Haas wrote:
>> On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> No, because then you get the cross-product of multiple SRFs, not the
>>> run-in-lockstep behavior.

>> Oh.  I assumed that was the expected behavior.  But, ah, what do I know?

> Lots, I assume -- but in this case, probably next to nothing, just like
> most of us, because what sane person or application would be really
> relying on the wacko historical behavior, in order to generate some
> collective knowledge?  However, I think that it is possible that
> someone, somewhere has two SRFs-in-targetlist that return the same
> number of rows and that the current implementation works fine for them;

Yes.  Run-in-lockstep is an extremely useful behavior, so much so that
we made a LATERAL variant for it.  I do not see a reason to break such
cases in the targetlist.

> My vote is to raise an error in the case of more than one SRF in targetlist.

Note that that risks breaking cases that the user does not think are "more
than one SRF".  Consider this example using a regression-test table:

regression=# create function foo() returns setof int8_tbl as
regression-# 'select * from int8_tbl' language sql;
CREATE FUNCTION
regression=# select foo();
                 foo                  
--------------------------------------
 (123,456)
 (123,4567890123456789)
 (4567890123456789,123)
 (4567890123456789,4567890123456789)
 (4567890123456789,-4567890123456789)
(5 rows)

regression=# explain verbose select foo();
                  QUERY PLAN                  
----------------------------------------------
 Result  (cost=0.00..5.25 rows=1000 width=32)
   Output: foo()
(2 rows)

regression=# select (foo()).*;
        q1        |        q2         
------------------+-------------------
              123 |               456
              123 |  4567890123456789
 4567890123456789 |               123
 4567890123456789 |  4567890123456789
 4567890123456789 | -4567890123456789
(5 rows)

regression=# explain verbose select (foo()).*;
                  QUERY PLAN                  
----------------------------------------------
 Result  (cost=0.00..5.50 rows=1000 width=16)
   Output: (foo()).q1, (foo()).q2
(2 rows)

The reason we can get away with this simplistic treatment of
composite-returning SRFs is precisely the run-in-lockstep behavior.
Otherwise the second query would have returned 25 rows.

Now, if we decide to try to rewrite tlist SRFs as LATERAL, it would likely
behoove us to do that rewrite before expanding * not after, so that we can
eliminate the multiple evaluation of foo() that happens currently.  (That
makes it a parser problem not a planner problem.)  And maybe we should
rewrite non-SRF composite-returning functions this way too, because people
have definitely complained about the extra evaluations in that context.
But my point here is that lockstep evaluation does have practical use
when the SRFs are iterating over matching collections of generated rows.
And that seems like a pretty common use-case.

			regards, tom lane

  43. Re: Changed SRF in targetlist handling

    Robert Haas <robertmhaas@gmail.com> — 2016-06-06T19:06:37Z

    On Mon, Jun 6, 2016 at 2:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Now, if we decide to try to rewrite tlist SRFs as LATERAL, it would likely
> behoove us to do that rewrite before expanding * not after, so that we can
> eliminate the multiple evaluation of foo() that happens currently.  (That
> makes it a parser problem not a planner problem.)  And maybe we should
> rewrite non-SRF composite-returning functions this way too, because people
> have definitely complained about the extra evaluations in that context.
> But my point here is that lockstep evaluation does have practical use
> when the SRFs are iterating over matching collections of generated rows.
> And that seems like a pretty common use-case.

Yeah, OK.  I'm not terribly opposed to going that way.  I think the
current behavior sucks badly enough - both because the semantics are
bizarre and because it complicates the whole executor for a niche
feature - that it's worth taking a backward compatibility hit to
change it.  I guess I'd prefer #2 to #2.5, #2.5 to #3, and #3 to #1.
I really don't like #1 much - I think I'd almost rather do nothing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  44. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-06-06T19:26:41Z

    Robert Haas <robertmhaas@gmail.com> writes:
> ... I guess I'd prefer #2 to #2.5, #2.5 to #3, and #3 to #1.
> I really don't like #1 much - I think I'd almost rather do nothing.

FWIW, that's about my evaluation of the alternatives as well.  I fear
that #1 would get a lot of pushback.  If we think that something like
"LATERAL ROWS FROM STRICT" is worth having on its own merits, then
doing #2.5 seems worthwhile to me, but otherwise I'm just as happy
with #2.  David J. seems to feel that throwing an error (as in #2.5)
rather than silently behaving incompatibly (as in #2) is important,
but I'm not convinced.  In a green field I think we'd prefer #2 over
#2.5, so I'd rather go that direction.

			regards, tom lane

  45. Re: Changed SRF in targetlist handling

    David G. Johnston <david.g.johnston@gmail.com> — 2016-06-06T20:43:06Z

    On Mon, Jun 6, 2016 at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Robert Haas <robertmhaas@gmail.com> writes:
> > ... I guess I'd prefer #2 to #2.5, #2.5 to #3, and #3 to #1.
> > I really don't like #1 much - I think I'd almost rather do nothing.
>
> FWIW, that's about my evaluation of the alternatives as well.  I fear
> that #1 would get a lot of pushback.  If we think that something like
> "LATERAL ROWS FROM STRICT" is worth having on its own merits, then
> doing #2.5 seems worthwhile to me, but otherwise I'm just as happy
> with #2.  David J. seems to feel that throwing an error (as in #2.5)
> rather than silently behaving incompatibly (as in #2) is important,
> but I'm not convinced.  In a green field I think we'd prefer #2 over
> #2.5, so I'd rather go that direction.
>

​I suspect the decision to error or not is a one or two line change in
whatever form the final patch takes.  It seems like approach #2 is
acceptable on a theoretical level which implies there is no desire to make
the existing LCM behavior available post-patch.

Assuming it is simple then everyone will have a chance to make their
opinion known on whether the 2.0 or 2.5 variation is preferable for the
final commit.  If a decision needs to be made sooner due to a design
decision I'd hope the author of the patch would make that known so we can
bring this to resolution at that point instead.

David J.

  46. Re: Changed SRF in targetlist handling

    Robert Haas <robertmhaas@gmail.com> — 2016-06-06T21:03:29Z

    On Mon, Jun 6, 2016 at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> ... I guess I'd prefer #2 to #2.5, #2.5 to #3, and #3 to #1.
>> I really don't like #1 much - I think I'd almost rather do nothing.
>
> FWIW, that's about my evaluation of the alternatives as well.  I fear
> that #1 would get a lot of pushback.  If we think that something like
> "LATERAL ROWS FROM STRICT" is worth having on its own merits, then
> doing #2.5 seems worthwhile to me, but otherwise I'm just as happy
> with #2.  David J. seems to feel that throwing an error (as in #2.5)
> rather than silently behaving incompatibly (as in #2) is important,
> but I'm not convinced.  In a green field I think we'd prefer #2 over
> #2.5, so I'd rather go that direction.

Same here.  That behavior is actually potentially quite useful, right?
 Like, you might want to rely on the NULL-extension thing, if it were
documented as behavior you can count on?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  47. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-08-01T08:23:46Z

    On 2016-05-25 16:55:23 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-05-25 15:20:03 -0400, Tom Lane wrote:
> >> We could certainly make a variant behavior in nodeFunctionscan.c that
> >> emulates that, if we feel that being exactly bug-compatible on the point
> >> is actually what we want.  I'm dubious about that though, not least
> >> because I don't think *anyone* actually believes that that behavior isn't
> >> broken.  Did you read my upthread message suggesting assorted compromise
> >> choices?
>
> > You mean https://www.postgresql.org/message-id/21076.1464034513@sss.pgh.pa.us ?
> > If so, yes.
>
> > If we go with rewriting this into LATERAL, I'd vote for 2.5 (trailed by
> > option 1), that'd keep most of the functionality, and would break
> > visibly rather than invisibly in the cases where not.
>
> 2.5 would be okay with me.
>
> > I guess you're not planning to work on this?
>
> Well, not right now, as it's clearly too late for 9.6.  I might hack on
> it later if nobody beats me to it.

FWIW, as it's blocking my plans for executor related rework (expression
evaluation, batch processing) I started to hack on this.

I've an implementation that

1) turns all targetlist SRF (tSRF from now on) into ROWS FROM
   expressions. If there's tSRFs in the argument of a tSRF those becomes
   a separate, lateral, ROWS FROM expression.

2) If grouping/window functions are present, the entire query is wrapped
   in a subquery RTE, except for the set-returning function. All
   referenced Var|Aggref|GroupingFunc|WindowFunc|Param nodes in the
   original targetlist are made to reference that subquery, which gets a
   TargetEntry for them.

3) If sortClause does *not* reference any tSRFs the sorting is evaluated
   in a subquery, to preserve the output ordering of SRFs in queries
   like
   SELECT id, generate_series(1,3) FROM (VALUES(1),(2)) d(id) ORDER BY id DESC;
   if in contrast sortClause does reference the tSRF output, it's
   evaluated in the outer SRF.

this seems to generally work, and allows to remove considerable amounts
of code.

So far I have one problem without an easy solution: Historically queries
like
=# SELECT id, generate_series(1,2) FROM (VALUES(1),(2)) few(id);
┌────┬─────────────────┐
│ id │ generate_series │
├────┼─────────────────┤
│  1 │               1 │
│  1 │               2 │
│  2 │               1 │
│  2 │               2 │
└────┴─────────────────┘
have preserved the SRF output ordering. But by turning the SRF into a
ROWS FROM, there's no guarantee that the cross join between "few" and
generate_series(1,3) above is implemented in that order. I.e. we can get
something like
┌────┬─────────────────┐
│ id │ generate_series │
├────┼─────────────────┤
│  1 │               1 │
│  2 │               1 │
│  1 │               2 │
│  2 │               2 │
└────┴─────────────────┘
because it's implemented as
┌──────────────────────────────────────────────────────────────────────────────┐
│                                  QUERY PLAN                                  │
├──────────────────────────────────────────────────────────────────────────────┤
│ Nested Loop  (cost=0.00..35.03 rows=2000 width=8)                            │
│   ->  Function Scan on generate_series  (cost=0.00..10.00 rows=1000 width=4) │
│   ->  Materialize  (cost=0.00..0.04 rows=2 width=4)                          │
│         ->  Values Scan on "*VALUES*"  (cost=0.00..0.03 rows=2 width=4)      │
└──────────────────────────────────────────────────────────────────────────────┘

I right now see no easy and nice-ish way to constrain that.


Besides that I'm structurally wondering whether turning the original
query into a subquery is the right thing to do. It requires some kind of
ugly munching of Query->*, and has the above problem. One alternative
would be to instead perform the necessary magic in grouping_planner(),
by "manually" adding nestloop joins before/after create_ordered_paths()
(depending on SRFs being referenced in the sort clause).  That'd create
plans we'd not have created so far, by layering NestLoop and
FunctionScan nodes above the normal query - that'd allow us to to easily
force the ordering of SRF evaluation.


If we go the subquery route, I'm wondering about where to tackle the
restructuring. So far I'm doing it very early in subquery_planner() -
otherwise the aggregation/sorting/... behaviour is easier to handle.
Perhaps doing it in standard_planner() itself would be better though.
An alternative approach would be to do this during parse-analysis, but I
think that might end up being confusing, because stored rules would
suddenly have a noticeably different structure, and it'd tie us a lot
more to the details of that transformation than I'd like.


Besides the removal of the least-common-multiple behaviour of tSRF queries,
there's some other consequences that using function scans have:
Previously if a tSRF was never evaluated, it didn't cause the number of
rows from being increased. E.g.
SELECT id, COALESCE(1, generate_series(1,2)) FROM (VALUES(1),(2)) few(id);
only produced two rows.  But using joins means that a simple
implementation of using ROWS FROM returns four rows.   We could try to
inject sufficient join conditions in that type of query, to prune down
the number of rows again, but I really don't want to go there - it's
kinda hard in the general case...


Comments?


Regards,


Andres

  48. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-08-02T23:02:38Z

    Andres Freund <andres@anarazel.de> writes:
> I've an implementation that

> 1) turns all targetlist SRF (tSRF from now on) into ROWS FROM
>    expressions. If there's tSRFs in the argument of a tSRF those becomes
>    a separate, lateral, ROWS FROM expression.

> 2) If grouping/window functions are present, the entire query is wrapped
>    in a subquery RTE, except for the set-returning function. All
>    referenced Var|Aggref|GroupingFunc|WindowFunc|Param nodes in the
>    original targetlist are made to reference that subquery, which gets a
>    TargetEntry for them.

FWIW, I'd be inclined to do the subquery RTE all the time, adding some
optimization fence to ensure it doesn't get folded back.  That fixes
your problem here:

> So far I have one problem without an easy solution: Historically queries
> like
> =# SELECT id, generate_series(1,2) FROM (VALUES(1),(2)) few(id);
> 
>  id  generate_series 
> 
>   1                1 
>   1                2 
>   2                1 
>   2                2 
> 
> have preserved the SRF output ordering. But by turning the SRF into a
> ROWS FROM, there's no guarantee that the cross join between "few" and
> generate_series(1,3) above is implemented in that order.


> Besides that I'm structurally wondering whether turning the original
> query into a subquery is the right thing to do. It requires some kind of
> ugly munching of Query->*, and has the above problem.

It does not seem like it should be that hard, certainly no worse than
subquery pullup.  Want to show code?

> An alternative approach would be to do this during parse-analysis, but I
> think that might end up being confusing, because stored rules would
> suddenly have a noticeably different structure, and it'd tie us a lot
> more to the details of that transformation than I'd like.

-1 on that; we do not want this transformation visible in stored rules.

> Besides the removal of the least-common-multiple behaviour of tSRF queries,
> there's some other consequences that using function scans have:
> Previously if a tSRF was never evaluated, it didn't cause the number of
> rows from being increased. E.g.
> SELECT id, COALESCE(1, generate_series(1,2)) FROM (VALUES(1),(2)) few(id);
> only produced two rows.  But using joins means that a simple
> implementation of using ROWS FROM returns four rows.

Hmm.  I don't mind changing behavior in that sort of corner case.
If we're prepared to discard the LCM behavior, this seems at least
an order of magnitude less likely to be worth worrying about.

Having said that, I do seem to recall a bug report about misbehavior when
a SRF was present in just one arm of a CASE statement.  That would have
the same type of behavior as you describe here, and evidently there's at
least one person out there depending on it.

Would it be worth detecting SRFs below CASE/COALESCE/etc and throwing
an error?  It would be easier to sell throwing an error than silently
changing behavior, I think.

			regards, tom lane

  49. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-08-02T23:30:55Z

    On 2016-08-02 19:02:38 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I've an implementation that
>
> > 1) turns all targetlist SRF (tSRF from now on) into ROWS FROM
> >    expressions. If there's tSRFs in the argument of a tSRF those becomes
> >    a separate, lateral, ROWS FROM expression.
>
> > 2) If grouping/window functions are present, the entire query is wrapped
> >    in a subquery RTE, except for the set-returning function. All
> >    referenced Var|Aggref|GroupingFunc|WindowFunc|Param nodes in the
> >    original targetlist are made to reference that subquery, which gets a
> >    TargetEntry for them.
>
> FWIW, I'd be inclined to do the subquery RTE all the time,

Yea, that's what I ended up doing.


> adding some
> optimization fence to ensure it doesn't get folded back.  That fixes
> your problem here:

> > So far I have one problem without an easy solution: Historically queries
> > like
> > =# SELECT id, generate_series(1,2) FROM (VALUES(1),(2)) few(id);
> > ┌────┬─────────────────┐
> > │ id │ generate_series │
> > ├────┼─────────────────┤
> > │  1 │               1 │
> > │  1 │               2 │
> > │  2 │               1 │
> > │  2 │               2 │
> > └────┴─────────────────┘
> > have preserved the SRF output ordering. But by turning the SRF into a
> > ROWS FROM, there's no guarantee that the cross join between "few" and
> > generate_series(1,3) above is implemented in that order.

But I don't see how that fixes the above problem?  The join, on the
top-level because of aggregates, still can be implemented as
subquery join srf or as srf join subquery, with the different output order
that implies.  I've duct-taped together a solution for that, by forcing
the lateral machinery to always see a dependency from the SRF to the
subquery; but that probably needs a nicer fix than a RangeTblEntry->deps
field which is processed in extract_lateral_references() ;)


> > Besides that I'm structurally wondering whether turning the original
> > query into a subquery is the right thing to do. It requires some kind of
> > ugly munching of Query->*, and has the above problem.
>
> It does not seem like it should be that hard, certainly no worse than
> subquery pullup.  Want to show code?

It's not super hard, there's some stuff like pushing/not-pushing
various sortgrouprefs to the subquery. But I think we can live with it.

Let me clean up the code some, hope to have something today or tomorrow.


> > An alternative approach would be to do this during parse-analysis, but I
> > think that might end up being confusing, because stored rules would
> > suddenly have a noticeably different structure, and it'd tie us a lot
> > more to the details of that transformation than I'd like.
>
> -1 on that; we do not want this transformation visible in stored rules.

Agreed.


> > Besides the removal of the least-common-multiple behaviour of tSRF queries,
> > there's some other consequences that using function scans have:
> > Previously if a tSRF was never evaluated, it didn't cause the number of
> > rows from being increased. E.g.
> > SELECT id, COALESCE(1, generate_series(1,2)) FROM (VALUES(1),(2)) few(id);
> > only produced two rows.  But using joins means that a simple
> > implementation of using ROWS FROM returns four rows.
>
> Hmm.  I don't mind changing behavior in that sort of corner case.
> If we're prepared to discard the LCM behavior, this seems at least
> an order of magnitude less likely to be worth worrying about.

I think it's fine, and potentially less confusing.


> Would it be worth detecting SRFs below CASE/COALESCE/etc and throwing
> an error?  It would be easier to sell throwing an error than silently
> changing behavior, I think.

Hm. We could, but I think the new behaviour would actually make sense in
the long run. Interpreting the coalesce to run on the output of the SRF
doesn't seem bad to me.


I found another edgecase, which we need to make a decision about.
'record' returning SRFs can't be transformed easily into a ROWS
FROM. Consider e.g. the following from the regression tests:

create function array_to_set(anyarray) returns setof record as $$
  select i AS "index", $1[i] AS "value" from generate_subscripts($1, 1)  i
$$ language sql strict immutable;

select array_to_set(array['one', 'two']);
┌──────────────┐
│ array_to_set │
├──────────────┤
│ (1,one)      │
│ (2,two)      │
└──────────────┘
(2 rows)

which currently works. That currently can't be modeled as ROWS FROM()
directly, because that desperately wants to return the columns as
columns, which we can't do for 'record' returning things, because they
don't have defined columns.  For composite returning SRFs I've currently
implemented that by generating a ROWS() expression, but that doesn't
work for record.

So it seems like we need some, not necessarily user exposed, way of
making nodeFunctionscan.c return the return value as one datum.  One
way, as suggested by Andrew G. on IRC, was to interpret empty column
definition in ROWS FROM interpreted that way.

Greetings,

Andres Freund

  50. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-08-04T03:22:03Z

    On 2016-08-02 16:30:55 -0700, Andres Freund wrote:
> > > Besides that I'm structurally wondering whether turning the original
> > > query into a subquery is the right thing to do. It requires some kind of
> > > ugly munching of Query->*, and has the above problem.
> >
> > It does not seem like it should be that hard, certainly no worse than
> > subquery pullup.  Want to show code?
>
> It's not super hard, there's some stuff like pushing/not-pushing
> various sortgrouprefs to the subquery. But I think we can live with it.
>
> Let me clean up the code some, hope to have something today or
> tomorrow.

Here we go.  This *clearly* is a POC, not more.  But it mostly works.


0001 - adds some test, some of those change after the later patches
0002 - main SRF via ROWS FROM () implementation
0003 - Large patch removing now unused code. Most satisfying.


The interesting bit is obviously 0002. What it basically does is, at the beginning
of subquery_planner():
1) unsrfify:
   move the jointree into a subquery
2) unsrfify_reference_subquery_mutator:
   process the old targetlist to reference the new subquery. If a
   TargetEntry doesn't contain a set, it's entirely moved into the
   subquery. Otherwise all Vars/Aggrefs/... it references are moved to
   the subquery, and referenced in the outer query's target list.
3) unsrfify_implement_srfs_mutator:
   Replace set returning functions in the targetlist with references to
   a new FUNCTION RTE. All non-nested tSRFs are part of the same RTE
   (i.e. the least common multiple behaviour is gone). all tSRFs in
   arguments are implemented as another FUNCTION RTE.

I discovered that we allow SRFs in UPDATE target lists. It's not clear
to me what that's supposed to mean. Nor how exactly to implement that,
given expand_targetlist(). Right now that fails with the patch, because
it re-inserts Var's for the relation replaced by the subquery.

Note that I've not bothered to fix up the regression test output - I'm
certain that explain output and such will still change.

Biggest questions / tasks:
* General approach
* DML handling
* Operator implementation
* SETOF record handling
* correct handling of lateral dependency from RTE to subquery to force
  evaluation order, instead of my RangeTblEntry->deps hack.
* lot of cleanup

Comments?

Greetings,

Andres Freund

  51. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-08-18T00:41:28Z

    On 2016-08-03 20:22:03 -0700, Andres Freund wrote:
> On 2016-08-02 16:30:55 -0700, Andres Freund wrote:
> > > > Besides that I'm structurally wondering whether turning the original
> > > > query into a subquery is the right thing to do. It requires some kind of
> > > > ugly munching of Query->*, and has the above problem.
> > >
> > > It does not seem like it should be that hard, certainly no worse than
> > > subquery pullup.  Want to show code?
> >
> > It's not super hard, there's some stuff like pushing/not-pushing
> > various sortgrouprefs to the subquery. But I think we can live with it.
> >
> > Let me clean up the code some, hope to have something today or
> > tomorrow.
> 
> Here we go.  This *clearly* is a POC, not more.  But it mostly works.
> 
> 
> 0001 - adds some test, some of those change after the later patches
> 0002 - main SRF via ROWS FROM () implementation
> 0003 - Large patch removing now unused code. Most satisfying.
> 
> 
> The interesting bit is obviously 0002. What it basically does is, at the beginning
> of subquery_planner():
> 1) unsrfify:
>    move the jointree into a subquery
> 2) unsrfify_reference_subquery_mutator:
>    process the old targetlist to reference the new subquery. If a
>    TargetEntry doesn't contain a set, it's entirely moved into the
>    subquery. Otherwise all Vars/Aggrefs/... it references are moved to
>    the subquery, and referenced in the outer query's target list.
> 3) unsrfify_implement_srfs_mutator:
>    Replace set returning functions in the targetlist with references to
>    a new FUNCTION RTE. All non-nested tSRFs are part of the same RTE
>    (i.e. the least common multiple behaviour is gone). all tSRFs in
>    arguments are implemented as another FUNCTION RTE.
> 
> I discovered that we allow SRFs in UPDATE target lists. It's not clear
> to me what that's supposed to mean. Nor how exactly to implement that,
> given expand_targetlist(). Right now that fails with the patch, because
> it re-inserts Var's for the relation replaced by the subquery.
> 
> Note that I've not bothered to fix up the regression test output - I'm
> certain that explain output and such will still change.
> 
> Biggest questions / tasks:
> * General approach
> * DML handling
> * Operator implementation
> * SETOF record handling
> * correct handling of lateral dependency from RTE to subquery to force
>   evaluation order, instead of my RangeTblEntry->deps hack.
> * lot of cleanup
> 
> Comments?

Tom, do you think this is roughly going in the right direction? My plan
here is to develop two patches, to come before this:

a) Allow to avoid using a tuplestore for SRF_PERCALL SRFs in ROWS FROM -
   otherwise our performance would regress noticeably in some cases.
b) Allow ROWS FROM() to return SETOF RECORD type SRFs as one column,
   instead of expanded. That's important to be able move SETOF RECORD
   returning functions in the targetlist into ROWS FROM, which otherwise
   requires an explicit column list.

Greetings,

Andres Freund

  52. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-08-22T18:44:17Z

    On 2016-08-17 17:41:28 -0700, Andres Freund wrote:
> Tom, do you think this is roughly going in the right direction? My plan
> here is to develop two patches, to come before this:
> 
> a) Allow to avoid using a tuplestore for SRF_PERCALL SRFs in ROWS FROM -
>    otherwise our performance would regress noticeably in some cases.
> b) Allow ROWS FROM() to return SETOF RECORD type SRFs as one column,
>    instead of expanded. That's important to be able move SETOF RECORD
>    returning functions in the targetlist into ROWS FROM, which otherwise
>    requires an explicit column list.

I'm working on these. Atm ExecMakeTableFunctionResult() resides in
execQual.c - I'm inlining it into nodeFunctionscan.c now, because
there's no other callers, and having it separate seems to bring no
benefit.

Please speak soon up if you disagree.

Andres

  53. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-08-22T19:01:01Z

    Hi,

On 2016-05-23 09:26:03 +0800, Craig Ringer wrote:
> SRFs-in-tlist are a lot faster for lockstep iteration etc. They're also
> much simpler to write, though if the result result rowcount differs
> unexpectedly between the functions you get exciting and unexpected
> behaviour.
> 
> WITH ORDINALITY provides what I think is the last of the functionality
> needed to replace SRFs-in-from, but at a syntatactic complexity and
> performance cost. The following example demonstrates that, though it
> doesn't do anything that needs LATERAL etc. I'm aware the following aren't
> semantically identical if the rowcounts differ.

I think here you're just missing ROWS FROM (generate_series(..), generate_series(...))

Andres

  54. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-08-22T20:20:58Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-08-17 17:41:28 -0700, Andres Freund wrote:
>> Tom, do you think this is roughly going in the right direction?

I've not had time to look at this patch, I'm afraid.  If you still
want me to, I can make time in a day or so.

> I'm working on these. Atm ExecMakeTableFunctionResult() resides in
> execQual.c - I'm inlining it into nodeFunctionscan.c now, because
> there's no other callers, and having it separate seems to bring no
> benefit.

> Please speak soon up if you disagree.

I think ExecMakeTableFunctionResult was placed in execQual.c because
it seemed to belong there alongside the support for SRFs in tlists.
If that's going away then there's no good reason not to move the logic
to where it's used.

			regards, tom lane

  55. Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-08-22T21:40:23Z

    Hi,

as noted in [1] I started hacking on removing the current implementation
of SRFs in the targetlist (tSRFs henceforth). IM discussion brought the
need for a description of the problem, need and approach to light.

There are several reasons for wanting to get rid of tSRFs. The primary
ones in my opinion are that the current behaviour of several SRFs in one
targetlist is confusing, and that the implementation burden currently is
all over the executor.  Especially the latter is what is motivating me
working on this, because it blocks my work on making the executor faster
for queries involving significant amounts of tuples.  Batching is hard
if random places in the querytree can icnrease the number of tuples.

The basic idea, hinted at in several threads, is, at plan time, to convert a query like
SELECT generate_series(1, 10);
into
SELECT generate_series FROM ROWS FROM(generate_series(1, 10));

thereby avoiding the complications in the executor (c.f. execQual.c
handling of isDone/ExprMultipleResult and supporting code in many
executor nodes / node->*.ps.ps_TupFromTlist).

There are several design questions along the way:

1) How to deal with the least-common-multiple behaviour of tSRFs. E.g.
=# SELECT generate_series(1, 3), generate_series(1,2);
returning
┌─────────────────┬─────────────────┐
│ generate_series │ generate_series │
├─────────────────┼─────────────────┤
│               1 │               1 │
│               2 │               2 │
│               3 │               1 │
│               1 │               2 │
│               2 │               1 │
│               3 │               2 │
└─────────────────┴─────────────────┘
(6 rows)
but
=# SELECT generate_series(1, 3), generate_series(5,7);
returning
┌─────────────────┬─────────────────┐
│ generate_series │ generate_series │
├─────────────────┼─────────────────┤
│               1 │               5 │
│               2 │               6 │
│               3 │               7 │
└─────────────────┴─────────────────┘

discussion in this thread came, according to my reading, to the
conclusion that that behaviour is just confusing and that the ROWS FROM
behaviour of
=# SELECT * FROM ROWS FROM(generate_series(1, 3), generate_series(1,2));
┌─────────────────┬─────────────────┐
│ generate_series │ generate_series │
├─────────────────┼─────────────────┤
│               1 │               1 │
│               2 │               2 │
│               3 │          (null) │
└─────────────────┴─────────────────┘
(3 rows)

makes more sense.  We also discussed erroring out if two SRFs  return
differing amount of rows, but that seems not to be preferred so far. And
we can easily add it if we want.


2) A naive conversion to ROWS FROM, like in the example in the
   introductory paragraph, can change the output, when implemented as a
   join from ROWS FROM to the rest of the query, rather than the other
   way round. E.g.
=# EXPLAIN SELECT * FROM few, ROWS FROM(generate_series(1,10));
┌──────────────────────────────────────────────────────────────────────────────┐
│                                  QUERY PLAN                                  │
├──────────────────────────────────────────────────────────────────────────────┤
│ Nested Loop  (cost=0.00..36.03 rows=2000 width=8)                            │
│   ->  Function Scan on generate_series  (cost=0.00..10.00 rows=1000 width=4) │
│   ->  Materialize  (cost=0.00..1.03 rows=2 width=4)                          │
│         ->  Seq Scan on few  (cost=0.00..1.02 rows=2 width=4)                │
└──────────────────────────────────────────────────────────────────────────────┘
(4 rows)
=# SELECT * FROM few, ROWS FROM(generate_series(1,3));
┌────┬─────────────────┐
│ id │ generate_series │
├────┼─────────────────┤
│  1 │               1 │
│  2 │               1 │
│  1 │               2 │
│  2 │               2 │
│  1 │               3 │
│  2 │               3 │
└────┴─────────────────┘
(6 rows)
surely isn't what was intended.  So the join order needs to be enforced.

3) tSRFs are evaluated after GROUP BY, and window functions:
=# SELECT generate_series(1, count(*)) FROM (VALUES(1),(2),(10)) f;
┌─────────────────┐
│ generate_series │
├─────────────────┤
│               1 │
│               2 │
│               3 │
└─────────────────┘
which means we have to push the "original" query into a subquery, with
the ROWS FROM laterally referencing the subquery:
SELECT generate_series FROM (SELECT count(*) FROM (VALUES(1),(2),(10)) f) s, ROWS FROM (generate_series(1,s.count));

4) The evaluation order of tSRFs in combination with ORDER BY is a bit
   confusing. Namely tSRFs are implemented after ORDER BY has been
   evaluated, unless the ORDER BY references the SRF.
E.g.
=# SELECT few.id, generate_series FROM ROWS FROM(generate_series(1,3)),few ORDER BY few.id DESC;
might return
┌────┬─────────────────┐
│ id │ generate_series │
├────┼─────────────────┤
│ 24 │               3 │
│ 24 │               2 │
│ 24 │               1 │
..
instead of
┌────┬─────────────────┐
│ id │ generate_series │
├────┼─────────────────┤
│ 24 │               1 │
│ 24 │               2 │
│ 24 │               3 │
as before.

which means we'll sometimes have to push down the ORDER BY into the
subquery (when not referencing tSRFs, so they're evaluated first),
sometimes evaluate them on the outside (if tSRFs are referenced)

5) tSRFs can have tSRFs as argument, e.g.:
=# SELECT generate_series(1, generate_series(1,3));
┌─────────────────┐
│ generate_series │
├─────────────────┤
│               1 │
│               1 │
│               2 │
│               1 │
│               2 │
│               3 │
└─────────────────┘
that can quite easily be implemented by having the "nested" tSRF
evaluate as a separate ROWS FROM expression.

Which even allows us to implement the previously forbidden
=# SELECT generate_series(generate_series(1,3), generate_series(2,4));
ERROR:  0A000: functions and operators can take at most one set argument

- not that I think that's of great value ;)

6) SETOF record type functions cannot directly be used in ROWS FROM() -
   as ROWS FROM "expands" records returned by functions. When converting
   something like
CREATE OR REPLACE FUNCTION setof_record_sql() RETURNS SETOF record LANGUAGE sql AS $$SELECT 1 AS a, 2 AS b UNION ALL SELECT 1, 2;$$;
SELECT setof_record_sql();
   we don't have that available though.

The best way to handle that seems to be to introduce the ability for
ROWS FROM not to expand the record returned by a column.  I'm currently
thinking that something like ROWS FROM(setof_record_sql() AS ()) would
do the trick.  That'd also considerably simplify the handling of
functions returning known composite types - my current POC patch
generates a ROW(a,b,..) type expression for those.

I'm open to better syntax suggestions.

7) ROWS FROM () / functions in the FROM list are currently signifcantly
   slower than the equivalent in the target list (for SFRM_ValuePerCall
   SRFs at least):

=# COPY (SELECT generate_series(1,10000000)) TO '/dev/null';
COPY 10000000
Time: 1311.469 ms
=# COPY (SELECT * FROM generate_series(1,10000000)) TO '/dev/null';
LOG:  00000: temporary file: path "base/pgsql_tmp/pgsql_tmp702.0", size 140000000
LOCATION:  FileClose, fd.c:1484
COPY 10000000
Time: 2173.282 ms
for SRFM_Materialize SRFs there's no meaningufl difference:
CREATE FUNCTION plpgsql_generate_series(bigint, bigint) RETURNS SETOF bigint LANGUAGE plpgsql AS $$BEGIN RETURN QUERY SELECT generate_series($1, $2);END;$$;

=# COPY (SELECT plpgsql_generate_series(1,10000000)) TO '/dev/null';
LOG:  00000: temporary file: path "base/pgsql_tmp/pgsql_tmp702.2", size 180000000
COPY 10000000
Time: 3058.437 ms

=# COPY (SELECT * FROM plpgsql_generate_series(1,10000000)) TO '/dev/null';LOG:  00000: temporary file: path "base/pgsql_tmp/pgsql_tmp702.1", size 180000000
COPY 10000000
Time: 2964.661 ms

that makes sense, because nodeFunctionscan.c, via
ExecMakeTableFunctionResult, forces materialization of ValuePerCall
SRFs.

ISTM that we need should fix that by allowing ValuePerCall without
materialization, as long as EXEC_FLAG_BACKWARD isn't required.


I've implemented ([2]) a prototype of this. My basic approach is:

I) During parse-analysis, remember whether a query has any tSRFs
   (Query->hasTargetSRF). That avoids doing a useless pass over the
   query, if no tSRFs are present.
II) At the beginning of subquery_planner(), before doing any work
   operating on subqueries and such, implement SRFs if ->hasTargetSRF().
   (unsrfify() in the POC)
III) Unconditionally move the "current" query into a subquery. For that
   do a mutator pass over the query, replacing Vars/Aggrefs/... in the
   original targetlist with Var references to the new subquery.
   (unsrfify_reference_subquery_mutator() in the POC)
IV) Do a pass over the outer query's targetlist, and implement any tSRFs
   using a ROWS FROM() RTE (or multiple ones in case of nested tSRFs).
   (unsrfify_implement_srfs_mutator() in the POC)

that seems to mostly work well.

The behaviour changes this implies are:

a) Least-common-multiple behaviour, as in (1) above, is gone. I think
   that's good.

b) We currently allow tSRFs in UPDATE ... SET expressions. I don't
   actually know what that's supposed to mean. I'm inclined
   a;
=# CREATE TABLE blarg AS SELECT 1::int a;
SELECT 1
=# UPDATE blarg SET a = generate_series(2,3);
UPDATE 1
=# SELECT * FROM blarg ;
┌───┐
│ a │
├───┤
│ 2 │
└───┘
I'm inclined to think that that's a bad idea, and should rather be
forbidden.

c) COALESCE/CASE have, so far, shortcut tSRF expansion. E.g.
   SELECT id, COALESCE(1, generate_series(1,2)) FROM (VALUES(1),(2)) few(id);
   returns only two rows, despite the generate_series().  But by
   implementing the generate_series as a ROWS FROM, it'd return four.

I think that's ok.

d) Not a problem with the patch per-se, but I'm doubful that that's ok:
=# SELECT 1 ORDER BY generate_series(1, 10);
returns 10 rows ;) - maybe we should forbid that?

As the patch currently stands, the diffstat is
 56 files changed, 953 insertions(+), 1599 deletions(-)
which isn't bad. I'd guess that a few more lines are needed, but I'd
still bet it's a net negative code-wise.

Regards,

Andres Freund

[1] http://archives.postgresql.org/message-id/20160801082346.nfp2g7mg74alifdc%40alap3.anarazel.de
[2] http://archives.postgresql.org/message-id/20160804032203.jprhdkx273sqhksd%40alap3.anarazel.de

  56. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-08-22T21:43:33Z

    Hi,

On 2016-08-22 16:20:58 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-08-17 17:41:28 -0700, Andres Freund wrote:
> >> Tom, do you think this is roughly going in the right direction?
> 
> I've not had time to look at this patch, I'm afraid.  If you still
> want me to, I can make time in a day or so.

That'd greatly be appreciated. I think polishing the POC up to
committable patch will be a considerable amount of work, and I'd like
design feedback before that.


> > I'm working on these. Atm ExecMakeTableFunctionResult() resides in
> > execQual.c - I'm inlining it into nodeFunctionscan.c now, because
> > there's no other callers, and having it separate seems to bring no
> > benefit.
> 
> > Please speak soon up if you disagree.
> 
> I think ExecMakeTableFunctionResult was placed in execQual.c because
> it seemed to belong there alongside the support for SRFs in tlists.
> If that's going away then there's no good reason not to move the logic
> to where it's used.

Cool, then we agree.

Greetings,

Andres Freund

  57. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Gavin Flower <gavinflower@archidevsys.co.nz> — 2016-08-22T22:02:43Z

    On 23/08/16 09:40, Andres Freund wrote:
> Hi,
>
> as noted in [1] I started hacking on removing the current implementation
> of SRFs in the targetlist (tSRFs henceforth). IM discussion brought the
> need for a description of the problem, need and approach to light.
>
> There are several reasons for wanting to get rid of tSRFs. The primary
> ones in my opinion are that the current behaviour of several SRFs in one
> targetlist is confusing, and that the implementation burden currently is
> all over the executor.  Especially the latter is what is motivating me
> working on this, because it blocks my work on making the executor faster
> for queries involving significant amounts of tuples.  Batching is hard
> if random places in the querytree can icnrease the number of tuples.
>
> The basic idea, hinted at in several threads, is, at plan time, to convert a query like
> SELECT generate_series(1, 10);
> into
> SELECT generate_series FROM ROWS FROM(generate_series(1, 10));
>
> thereby avoiding the complications in the executor (c.f. execQual.c
> handling of isDone/ExprMultipleResult and supporting code in many
> executor nodes / node->*.ps.ps_TupFromTlist).
>
> There are several design questions along the way:
>
> 1) How to deal with the least-common-multiple behaviour of tSRFs. E.g.
> =# SELECT generate_series(1, 3), generate_series(1,2);
> returning
> ┌─────────────────┬─────────────────┐
> │ generate_series │ generate_series │
> ├─────────────────┼─────────────────┤
> │               1 │               1 │
> │               2 │               2 │
> │               3 │               1 │
> │               1 │               2 │
> │               2 │               1 │
> │               3 │               2 │
> └─────────────────┴─────────────────┘
> (6 rows)
> but
> =# SELECT generate_series(1, 3), generate_series(5,7);
> returning
> ┌─────────────────┬─────────────────┐
> │ generate_series │ generate_series │
> ├─────────────────┼─────────────────┤
> │               1 │               5 │
> │               2 │               6 │
> │               3 │               7 │
> └─────────────────┴─────────────────┘
>
> discussion in this thread came, according to my reading, to the
> conclusion that that behaviour is just confusing and that the ROWS FROM
> behaviour of
> =# SELECT * FROM ROWS FROM(generate_series(1, 3), generate_series(1,2));
> ┌─────────────────┬─────────────────┐
> │ generate_series │ generate_series │
> ├─────────────────┼─────────────────┤
> │               1 │               1 │
> │               2 │               2 │
> │               3 │          (null) │
> └─────────────────┴─────────────────┘
> (3 rows)
>
> makes more sense.
I had always implicitly assumed that having 2 generated sequences would 
act as equivalent to:

SELECT
     sa,
     sb
FROM
     ROWS FROM(generate_series(1, 3)) AS sa,
     ROWS FROM(generate_series(5, 7)) AS sb
ORDER BY
     sa,
     sb;

  sa | sb
----+----
   1 |  5
   1 |  6
   1 |  7
   2 |  5
   2 |  6
   2 |  7
   3 |  5
   3 |  6
   3 |  7


Obviously I was wrong - but to me, my implicit assumption makes more sense!
[...]


Cheers,
Gavin

  58. Re: Changed SRF in targetlist handling

    Andres Freund <andres@anarazel.de> — 2016-08-23T18:55:16Z

    On 2016-08-17 17:41:28 -0700, Andres Freund wrote:
> a) Allow to avoid using a tuplestore for SRF_PERCALL SRFs in ROWS FROM -
>    otherwise our performance would regress noticeably in some cases.

To demonstrate the problem:

master:
=# COPY (SELECT generate_series(1, 50000000)) TO '/dev/null';
COPY 50000000
Time: 6859.830 ms
=# COPY (SELECT * FROM generate_series(1, 50000000)) TO '/dev/null';
COPY 50000000
Time: 11314.507 ms

getting rid of the materialization indeed fixes the problem:

dev:
=# COPY (SELECT generate_series(1, 50000000)) TO
'/dev/null';
COPY 50000000
Time: 5757.547 ms

=# COPY (SELECT * FROM generate_series(1, 50000000)) TO
'/dev/null';
COPY 50000000
Time: 5842.524 ms

I've currently implemented this by having nodeFunctionscan.c store
enough state in FunctionScanPerFuncState to continue the ValuePerCall
protocol.  That all seems to work well, without big problems.

The open issue here is whether / how we want to deal with
EXEC_FLAG_REWIND and EXEC_FLAG_BACKWARD. Currently that, with some added
complications, is implemented in nodeFunctionscan.c itself. But for
ValuePerCall SRFs that doesn't directly work anymore.

ISTM that the easiest way here is actually to rip out support for
EXEC_FLAG_REWIND/EXEC_FLAG_BACKWARD from nodeFunctionscan.c. If the plan
requires that, the planner will slap a Material node on-top. Which will
even be more efficient when ROWS FROM for multiple SRFs, or WITH
ORDINALITY, are used.  Alternatively we can continue to create a
tuplestore for ValuePerCall when eflags indicates that's required. But
personally I don't see that as an advantageous course.

Comments?

Andres

  59. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-08-27T21:48:29Z

    Hi,

Attached is a significantly updated patch series (see the mail one up
for details about what this is, I don't want to quote it in its
entirety).

There's still some corner cases (DISTINCT + SRF, UNION/INTERSECT with
SRF) to test / implement and a good bit of code cleanup to do. But
feature wise it's pretty much complete.

It currently consists of the following patches:

0001-Add-some-more-targetlist-srf-tests.patch
  Add some test.

0002-Shore-up-some-weird-corner-cases-for-targetlist-SRFs.patch
  Forbid UPDATE ... SET foo = SRF() and ORDER BY / GROUP BY containing
  SRFs that would change the number of returned rows.  Without the
  latter e.g. SELECT 1 ORDER BY generate_series(1,10); returns 10 rows.

0003-Avoid-materializing-SRFs-in-the-FROM-list.patch
  To avoid performance regressions from moving SRFM_ValuePerCall SRFs to
  ROWS FROM, nodeFunctionscan.c needs to support not materializing
  output.

  In my present patch I've *ripped out* the support for materialization
  in nodeFunctionscan.c entirely. That means that rescans referencing
  volatile functions can change their behaviour (if a function is
  rescanned, without having it's parameters changed), and that native
  backward scan support is gone.  I don't think that's actually an issue.

  This temporarily duplicates a bit of code from execQual.c, but that's
  removed again in 0006.

0004-Allow-ROWS-FROM-to-return-functions-as-single-record.patch
  To allow transforming SELECT record_srf(); nodeFunctionscan.c needs to
  learn to return the result as a record.  I chose
  ROWS FROM (record_srf() AS ()) as the syntax for that. It doesn't
  necessarily have to be SQL exposed, but it does make testing easier.

0005-Basic-implementation-of-targetlist-SRFs-via-ROWS-FRO.patch
  Convert all targetlist SRFs to ROWS FROM() expression, referencing the
  original query (without the SRFs) via a subquery.

  Note that this changes the behaviour of queries in a few cases. Namely
  the "least-common-multiple" behaviour of targetlist SRFs is gone, a
  function can now accept multiple set returning functions as input, SRF
  references in COALESCE / CASE are evaluated a bit more "eagerly". The
  last one I have to think a bit more about.

0006-Remove-unused-code-related-to-targetlist-SRFs.patch
  Now that there's no targetlist SRFs at execution time anymore, rip out
  executor and planner code related to that.  There's possibly more, but
  that's what I could find in a couple passes of searching around.

  This actually speeds up tpch-h queries by roughly 4% for me.


My next steps are to work on cleaning up the code a bit more, and
increase the regression coverage.

Input is greatly welcome.

Greetings,

Andres Freund

  60. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-08-28T03:38:04Z

    On 2016-08-27 14:48:29 -0700, Andres Freund wrote:
> My next steps are to work on cleaning up the code a bit more, and
> increase the regression coverage.

Oh, there's one open item I actually don't really know how to handle
well: A decent way of enforcing the join order between the subquery and
the functionscan when there's no lateral dependencies.  I've hacked up
the lateral machinery to just always add a pointless dependency, but
that seems fairly ugly.  If somebody has a better idea, that'd be great.

Greetings,

Andres Freund

  61. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Heikki Linnakangas <hlinnaka@iki.fi> — 2016-08-29T09:56:25Z

    On 08/28/2016 12:48 AM, Andres Freund wrote:
> Attached is a significantly updated patch series (see the mail one up
> for details about what this is, I don't want to quote it in its
> entirety).
>
> There's still some corner cases (DISTINCT + SRF, UNION/INTERSECT with
> SRF) to test / implement and a good bit of code cleanup to do. But
> feature wise it's pretty much complete.

Looks good, aside from the few FIXMEs, TODOs and XXXs and DIRTYs.

I think we need to come up with a better word for "unsrfify". That's 
quite a mouthful. Perhaps something as boring as 
"convert_srfs_to_function_rtes".

Would it make sense for addRangeTableEntryForFunction() to take  a List 
of RangeFunctionElems as argument, now that we have such a struct? The 
lists-of-same-length approach gets a bit tedious.

Typos:
s/fortfour/forfour
s/Each element of this list a/ Each element of this list is a/

- Heikki

  62. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-08-29T15:48:56Z

    On 2016-08-29 12:56:25 +0300, Heikki Linnakangas wrote:
> On 08/28/2016 12:48 AM, Andres Freund wrote:
> > Attached is a significantly updated patch series (see the mail one up
> > for details about what this is, I don't want to quote it in its
> > entirety).
> > 
> > There's still some corner cases (DISTINCT + SRF, UNION/INTERSECT with
> > SRF) to test / implement and a good bit of code cleanup to do. But
> > feature wise it's pretty much complete.
> 
> Looks good

Thanks for the look!


> aside from the few FIXMEs, TODOs and XXXs

Those I pretty much know to handle.


> DIRTYs.

But I think this one is the "ordering" dependency information, and there
I don't yet have good idea.


> I think we need to come up with a better word for "unsrfify". That's quite a
> mouthful. Perhaps something as boring as "convert_srfs_to_function_rtes".

Yea, that was more of a working title. Maybe implement_targetlist_srfs()?


> Would it make sense for addRangeTableEntryForFunction() to take  a List of
> RangeFunctionElems as argument, now that we have such a struct? The
> lists-of-same-length approach gets a bit tedious.

Yea, I was thinking the same.


> Typos:
> s/fortfour/forfour
> s/Each element of this list a/ Each element of this list is a/

Thanks.

Greetings,

Andres Freund

  63. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Robert Haas <robertmhaas@gmail.com> — 2016-09-02T08:31:32Z

    On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres@anarazel.de> wrote:
> as noted in [1] I started hacking on removing the current implementation
> of SRFs in the targetlist (tSRFs henceforth). IM discussion brought the
> need for a description of the problem, need and approach to light.

Thanks for writing this up.

> 1) How to deal with the least-common-multiple behaviour of tSRFs. E.g.
> =# SELECT generate_series(1, 3), generate_series(1,2);
> returning
> ┌─────────────────┬─────────────────┐
> │ generate_series │ generate_series │
> ├─────────────────┼─────────────────┤
> │               1 │               1 │
> │               2 │               2 │
> │               3 │               1 │
> │               1 │               2 │
> │               2 │               1 │
> │               3 │               2 │
> └─────────────────┴─────────────────┘
> (6 rows)
> but
> =# SELECT generate_series(1, 3), generate_series(5,7);
> returning
> ┌─────────────────┬─────────────────┐
> │ generate_series │ generate_series │
> ├─────────────────┼─────────────────┤
> │               1 │               5 │
> │               2 │               6 │
> │               3 │               7 │
> └─────────────────┴─────────────────┘
>
> discussion in this thread came, according to my reading, to the
> conclusion that that behaviour is just confusing and that the ROWS FROM
> behaviour of
> =# SELECT * FROM ROWS FROM(generate_series(1, 3), generate_series(1,2));
> ┌─────────────────┬─────────────────┐
> │ generate_series │ generate_series │
> ├─────────────────┼─────────────────┤
> │               1 │               1 │
> │               2 │               2 │
> │               3 │          (null) │
> └─────────────────┴─────────────────┘
> (3 rows)
>
> makes more sense.  We also discussed erroring out if two SRFs  return
> differing amount of rows, but that seems not to be preferred so far. And
> we can easily add it if we want.

This all seems fine.  I don't think erroring out is an improvement.

> 2) A naive conversion to ROWS FROM, like in the example in the
>    introductory paragraph, can change the output, when implemented as a
>    join from ROWS FROM to the rest of the query, rather than the other
>    way round. E.g.
> =# EXPLAIN SELECT * FROM few, ROWS FROM(generate_series(1,10));
> ┌──────────────────────────────────────────────────────────────────────────────┐
> │                                  QUERY PLAN                                  │
> ├──────────────────────────────────────────────────────────────────────────────┤
> │ Nested Loop  (cost=0.00..36.03 rows=2000 width=8)                            │
> │   ->  Function Scan on generate_series  (cost=0.00..10.00 rows=1000 width=4) │
> │   ->  Materialize  (cost=0.00..1.03 rows=2 width=4)                          │
> │         ->  Seq Scan on few  (cost=0.00..1.02 rows=2 width=4)                │
> └──────────────────────────────────────────────────────────────────────────────┘
> (4 rows)
> =# SELECT * FROM few, ROWS FROM(generate_series(1,3));
> ┌────┬─────────────────┐
> │ id │ generate_series │
> ├────┼─────────────────┤
> │  1 │               1 │
> │  2 │               1 │
> │  1 │               2 │
> │  2 │               2 │
> │  1 │               3 │
> │  2 │               3 │
> └────┴─────────────────┘
> (6 rows)
> surely isn't what was intended.  So the join order needs to be enforced.

In general, we've been skeptical about giving any guarantees about
result ordering.  Maybe this case is different and we should give some
guarantee here, but I don't think it's 100% obvious.

> 3) tSRFs are evaluated after GROUP BY, and window functions:
> =# SELECT generate_series(1, count(*)) FROM (VALUES(1),(2),(10)) f;
> ┌─────────────────┐
> │ generate_series │
> ├─────────────────┤
> │               1 │
> │               2 │
> │               3 │
> └─────────────────┘
> which means we have to push the "original" query into a subquery, with
> the ROWS FROM laterally referencing the subquery:
> SELECT generate_series FROM (SELECT count(*) FROM (VALUES(1),(2),(10)) f) s, ROWS FROM (generate_series(1,s.count));

Seems OK.

> 4) The evaluation order of tSRFs in combination with ORDER BY is a bit
>    confusing. Namely tSRFs are implemented after ORDER BY has been
>    evaluated, unless the ORDER BY references the SRF.
> E.g.
> =# SELECT few.id, generate_series FROM ROWS FROM(generate_series(1,3)),few ORDER BY few.id DESC;
> might return
> ┌────┬─────────────────┐
> │ id │ generate_series │
> ├────┼─────────────────┤
> │ 24 │               3 │
> │ 24 │               2 │
> │ 24 │               1 │
> ..
> instead of
> ┌────┬─────────────────┐
> │ id │ generate_series │
> ├────┼─────────────────┤
> │ 24 │               1 │
> │ 24 │               2 │
> │ 24 │               3 │
> as before.
>
> which means we'll sometimes have to push down the ORDER BY into the
> subquery (when not referencing tSRFs, so they're evaluated first),
> sometimes evaluate them on the outside (if tSRFs are referenced)

OK.

> 5) tSRFs can have tSRFs as argument, e.g.:
> =# SELECT generate_series(1, generate_series(1,3));
> ┌─────────────────┐
> │ generate_series │
> ├─────────────────┤
> │               1 │
> │               1 │
> │               2 │
> │               1 │
> │               2 │
> │               3 │
> └─────────────────┘
> that can quite easily be implemented by having the "nested" tSRF
> evaluate as a separate ROWS FROM expression.
>
> Which even allows us to implement the previously forbidden
> =# SELECT generate_series(generate_series(1,3), generate_series(2,4));
> ERROR:  0A000: functions and operators can take at most one set argument
>
> - not that I think that's of great value ;)

OK.

> 6) SETOF record type functions cannot directly be used in ROWS FROM() -
>    as ROWS FROM "expands" records returned by functions. When converting
>    something like
> CREATE OR REPLACE FUNCTION setof_record_sql() RETURNS SETOF record LANGUAGE sql AS $$SELECT 1 AS a, 2 AS b UNION ALL SELECT 1, 2;$$;
> SELECT setof_record_sql();
>    we don't have that available though.
>
> The best way to handle that seems to be to introduce the ability for
> ROWS FROM not to expand the record returned by a column.  I'm currently
> thinking that something like ROWS FROM(setof_record_sql() AS ()) would
> do the trick.  That'd also considerably simplify the handling of
> functions returning known composite types - my current POC patch
> generates a ROW(a,b,..) type expression for those.
>
> I'm open to better syntax suggestions.

I definitely agree that having some syntax to avoid row-expansion in
this case (and maybe in other cases) would be a good thing; I suspect
that would get a good bit of use.  I don't care much for that
particular choice of syntax, which seems fairly magical, but I'm not
sure what would be better.

> 7) ROWS FROM () / functions in the FROM list are currently signifcantly
>    slower than the equivalent in the target list (for SFRM_ValuePerCall
>    SRFs at least):
>
> =# COPY (SELECT generate_series(1,10000000)) TO '/dev/null';
> COPY 10000000
> Time: 1311.469 ms
> =# COPY (SELECT * FROM generate_series(1,10000000)) TO '/dev/null';
> LOG:  00000: temporary file: path "base/pgsql_tmp/pgsql_tmp702.0", size 140000000
> LOCATION:  FileClose, fd.c:1484
> COPY 10000000
> Time: 2173.282 ms
> for SRFM_Materialize SRFs there's no meaningufl difference:
> CREATE FUNCTION plpgsql_generate_series(bigint, bigint) RETURNS SETOF bigint LANGUAGE plpgsql AS $$BEGIN RETURN QUERY SELECT generate_series($1, $2);END;$$;
>
> =# COPY (SELECT plpgsql_generate_series(1,10000000)) TO '/dev/null';
> LOG:  00000: temporary file: path "base/pgsql_tmp/pgsql_tmp702.2", size 180000000
> COPY 10000000
> Time: 3058.437 ms
>
> =# COPY (SELECT * FROM plpgsql_generate_series(1,10000000)) TO '/dev/null';LOG:  00000: temporary file: path "base/pgsql_tmp/pgsql_tmp702.1", size 180000000
> COPY 10000000
> Time: 2964.661 ms
>
> that makes sense, because nodeFunctionscan.c, via
> ExecMakeTableFunctionResult, forces materialization of ValuePerCall
> SRFs.
>
> ISTM that we need should fix that by allowing ValuePerCall without
> materialization, as long as EXEC_FLAG_BACKWARD isn't required.

That sounds good.
> I've implemented ([2]) a prototype of this. My basic approach is:
>
> I) During parse-analysis, remember whether a query has any tSRFs
>    (Query->hasTargetSRF). That avoids doing a useless pass over the
>    query, if no tSRFs are present.
> II) At the beginning of subquery_planner(), before doing any work
>    operating on subqueries and such, implement SRFs if ->hasTargetSRF().
>    (unsrfify() in the POC)
> III) Unconditionally move the "current" query into a subquery. For that
>    do a mutator pass over the query, replacing Vars/Aggrefs/... in the
>    original targetlist with Var references to the new subquery.
>    (unsrfify_reference_subquery_mutator() in the POC)
> IV) Do a pass over the outer query's targetlist, and implement any tSRFs
>    using a ROWS FROM() RTE (or multiple ones in case of nested tSRFs).
>    (unsrfify_implement_srfs_mutator() in the POC)
>
> that seems to mostly work well.

I gather that III and IV are skipped if hasTargetSRF isn't set.

> The behaviour changes this implies are:
>
> a) Least-common-multiple behaviour, as in (1) above, is gone. I think
>    that's good.
>
> b) We currently allow tSRFs in UPDATE ... SET expressions. I don't
>    actually know what that's supposed to mean. I'm inclined
>    a;
> =# CREATE TABLE blarg AS SELECT 1::int a;
> SELECT 1
> =# UPDATE blarg SET a = generate_series(2,3);
> UPDATE 1
> =# SELECT * FROM blarg ;
> ┌───┐
> │ a │
> ├───┤
> │ 2 │
> └───┘
> I'm inclined to think that that's a bad idea, and should rather be
> forbidden.
>
> c) COALESCE/CASE have, so far, shortcut tSRF expansion. E.g.
>    SELECT id, COALESCE(1, generate_series(1,2)) FROM (VALUES(1),(2)) few(id);
>    returns only two rows, despite the generate_series().  But by
>    implementing the generate_series as a ROWS FROM, it'd return four.
>
> I think that's ok.

Those all sound OK.

> d) Not a problem with the patch per-se, but I'm doubful that that's ok:
> =# SELECT 1 ORDER BY generate_series(1, 10);
> returns 10 rows ;) - maybe we should forbid that?

OK by me.  I feel like this isn't the only case where the presence of
resjunk columns has user-visible effects, although I can't think of
another one right at the moment.  It seems like something to avoid,
though.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  64. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Robert Haas <robertmhaas@gmail.com> — 2016-09-02T08:34:24Z

    On Sun, Aug 28, 2016 at 3:18 AM, Andres Freund <andres@anarazel.de> wrote:
> 0003-Avoid-materializing-SRFs-in-the-FROM-list.patch
>   To avoid performance regressions from moving SRFM_ValuePerCall SRFs to
>   ROWS FROM, nodeFunctionscan.c needs to support not materializing
>   output.
>
>   In my present patch I've *ripped out* the support for materialization
>   in nodeFunctionscan.c entirely. That means that rescans referencing
>   volatile functions can change their behaviour (if a function is
>   rescanned, without having it's parameters changed), and that native
>   backward scan support is gone.  I don't think that's actually an issue.

Can you expand on why you think those things aren't an issue?  Because
it seems like they might be.

> 0006-Remove-unused-code-related-to-targetlist-SRFs.patch
>   Now that there's no targetlist SRFs at execution time anymore, rip out
>   executor and planner code related to that.  There's possibly more, but
>   that's what I could find in a couple passes of searching around.
>
>   This actually speeds up tpch-h queries by roughly 4% for me.

Nice.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  65. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Kevin Grittner <kgrittn@gmail.com> — 2016-09-02T14:05:35Z

    On Fri, Sep 2, 2016 at 3:31 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres@anarazel.de> wrote:

>> =# SELECT * FROM few, ROWS FROM(generate_series(1,3));
>> ┌────┬─────────────────┐
>> │ id │ generate_series │
>> ├────┼─────────────────┤
>> │  1 │               1 │
>> │  2 │               1 │
>> │  1 │               2 │
>> │  2 │               2 │
>> │  1 │               3 │
>> │  2 │               3 │
>> └────┴─────────────────┘
>> (6 rows)
>> surely isn't what was intended.  So the join order needs to be enforced.
>
> In general, we've been skeptical about giving any guarantees about
> result ordering.

+1

I think it is a very bad idea to move away from the statement that
a query generates a set of rows, and that no order is guaranteed
unless the top level has an ORDER BY clause.  How hard is it to add
ORDER BY 1, 2 to the above query?  Let the optimizer notice when a
node returns data in the needed order and skip the sort if possible.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  66. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-02T14:11:10Z

    On 2016-09-02 09:05:35 -0500, Kevin Grittner wrote:
> On Fri, Sep 2, 2016 at 3:31 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres@anarazel.de> wrote:
> 
> >> =# SELECT * FROM few, ROWS FROM(generate_series(1,3));
> >> ┌────┬─────────────────┐
> >> │ id │ generate_series │
> >> ├────┼─────────────────┤
> >> │  1 │               1 │
> >> │  2 │               1 │
> >> │  1 │               2 │
> >> │  2 │               2 │
> >> │  1 │               3 │
> >> │  2 │               3 │
> >> └────┴─────────────────┘
> >> (6 rows)
> >> surely isn't what was intended.  So the join order needs to be enforced.
> >
> > In general, we've been skeptical about giving any guarantees about
> > result ordering.

Well, it's historically how we behaved for SRFs. I'm pretty sure that
people will be confused if
SELECT generate_series(1, 10) FROM sometbl;
suddenly returns rows in an order that reverse from what
generate_series() returns.

> +
> 
> I think it is a very bad idea to move away from the statement that
> a query generates a set of rows, and that no order is guaranteed
> unless the top level has an ORDER BY clause.  How hard is it to add
> ORDER BY 1, 2 to the above query?  Let the optimizer notice when a
> node returns data in the needed order and skip the sort if possible.

There's no such infrastructure for SRFS/ROWS FROM.

Andres

  67. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-02T14:15:47Z

    Hi,

Thanks for looking.

On 2016-09-02 14:01:32 +0530, Robert Haas wrote:
> > 6) SETOF record type functions cannot directly be used in ROWS FROM() -
> >    as ROWS FROM "expands" records returned by functions. When converting
> >    something like
> > CREATE OR REPLACE FUNCTION setof_record_sql() RETURNS SETOF record LANGUAGE sql AS $$SELECT 1 AS a, 2 AS b UNION ALL SELECT 1, 2;$$;
> > SELECT setof_record_sql();
> >    we don't have that available though.
> >
> > The best way to handle that seems to be to introduce the ability for
> > ROWS FROM not to expand the record returned by a column.  I'm currently
> > thinking that something like ROWS FROM(setof_record_sql() AS ()) would
> > do the trick.  That'd also considerably simplify the handling of
> > functions returning known composite types - my current POC patch
> > generates a ROW(a,b,..) type expression for those.
> >
> > I'm open to better syntax suggestions.
> 
> I definitely agree that having some syntax to avoid row-expansion in
> this case (and maybe in other cases) would be a good thing; I suspect
> that would get a good bit of use.  I don't care much for that
> particular choice of syntax, which seems fairly magical, but I'm not
> sure what would be better.

I'm not a fan either, but until somebody ocmes up with something better
:/

> That sounds good.
> > I've implemented ([2]) a prototype of this. My basic approach is:
> >
> > I) During parse-analysis, remember whether a query has any tSRFs
> >    (Query->hasTargetSRF). That avoids doing a useless pass over the
> >    query, if no tSRFs are present.
> > II) At the beginning of subquery_planner(), before doing any work
> >    operating on subqueries and such, implement SRFs if ->hasTargetSRF().
> >    (unsrfify() in the POC)
> > III) Unconditionally move the "current" query into a subquery. For that
> >    do a mutator pass over the query, replacing Vars/Aggrefs/... in the
> >    original targetlist with Var references to the new subquery.
> >    (unsrfify_reference_subquery_mutator() in the POC)
> > IV) Do a pass over the outer query's targetlist, and implement any tSRFs
> >    using a ROWS FROM() RTE (or multiple ones in case of nested tSRFs).
> >    (unsrfify_implement_srfs_mutator() in the POC)
> >
> > that seems to mostly work well.
> 
> I gather that III and IV are skipped if hasTargetSRF isn't set.

Precisely.


> > d) Not a problem with the patch per-se, but I'm doubful that that's ok:
> > =# SELECT 1 ORDER BY generate_series(1, 10);
> > returns 10 rows ;) - maybe we should forbid that?
> 
> OK by me.  I feel like this isn't the only case where the presence of
> resjunk columns has user-visible effects, although I can't think of
> another one right at the moment.  It seems like something to avoid,
> though.

An early patch in the series now errors out if ORDER BY or GROUP BY adds
a retset resjunk element.


Regards,

Andres

  68. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Kevin Grittner <kgrittn@gmail.com> — 2016-09-02T14:18:38Z

    On Fri, Sep 2, 2016 at 9:11 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-09-02 09:05:35 -0500, Kevin Grittner wrote:
>> On Fri, Sep 2, 2016 at 3:31 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres@anarazel.de> wrote:
>>
>>>> =# SELECT * FROM few, ROWS FROM(generate_series(1,3));
>>>> ┌────┬─────────────────┐
>>>> │ id │ generate_series │
>>>> ├────┼─────────────────┤
>>>> │  1 │               1 │
>>>> │  2 │               1 │
>>>> │  1 │               2 │
>>>> │  2 │               2 │
>>>> │  1 │               3 │
>>>> │  2 │               3 │
>>>> └────┴─────────────────┘
>>>> (6 rows)
>>>> surely isn't what was intended.  So the join order needs to be enforced.
>>>
>>> In general, we've been skeptical about giving any guarantees about
>>> result ordering.
>
> Well, it's historically how we behaved for SRFs.

And until we had synchronized scans a sequential scan always
returned rows in the order they were present in the heap.
Implementation details are not guarantees.

> I'm pretty sure that people will be confused if
> SELECT generate_series(1, 10) FROM sometbl;
> suddenly returns rows in an order that reverse from what
> generate_series() returns.

If this changes, it is probably worth a mentioning in the release
notes.

>> I think it is a very bad idea to move away from the statement that
>> a query generates a set of rows, and that no order is guaranteed
>> unless the top level has an ORDER BY clause.  How hard is it to add
>> ORDER BY 1, 2 to the above query?  Let the optimizer notice when a
>> node returns data in the needed order and skip the sort if possible.
>
> There's no such infrastructure for SRFS/ROWS FROM.

Well, that's something to fix (or not), but not a justification for
"except on Tuesdays when the moon is full" sorts of exceptions to
simple rules about what to expect.  No ORDER BY means no order
guaranteed.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  69. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-02T14:20:23Z

    On 2016-09-02 07:11:10 -0700, Andres Freund wrote:
> On 2016-09-02 09:05:35 -0500, Kevin Grittner wrote:
> > On Fri, Sep 2, 2016 at 3:31 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > > On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres@anarazel.de> wrote:
> > 
> > >> =# SELECT * FROM few, ROWS FROM(generate_series(1,3));
> > >> ┌────┬─────────────────┐
> > >> │ id │ generate_series │
> > >> ├────┼─────────────────┤
> > >> │  1 │               1 │
> > >> │  2 │               1 │
> > >> │  1 │               2 │
> > >> │  2 │               2 │
> > >> │  1 │               3 │
> > >> │  2 │               3 │
> > >> └────┴─────────────────┘
> > >> (6 rows)
> > >> surely isn't what was intended.  So the join order needs to be enforced.
> > >
> > > In general, we've been skeptical about giving any guarantees about
> > > result ordering.
> 
> Well, it's historically how we behaved for SRFs. I'm pretty sure that
> people will be confused if
> SELECT generate_series(1, 10) FROM sometbl;
> suddenly returns rows in an order that reverse from what
> generate_series() returns.

Oh, and we've previously re-added that based on
complaints. C.f. d543170f2fdd6d9845aaf91dc0f6be7a2bf0d9e7 (and others
IIRC).

  70. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-02T14:20:42Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-02 09:05:35 -0500, Kevin Grittner wrote:
>>> In general, we've been skeptical about giving any guarantees about
>>> result ordering.

> Well, it's historically how we behaved for SRFs. I'm pretty sure that
> people will be confused if
> SELECT generate_series(1, 10) FROM sometbl;
> suddenly returns rows in an order that reverse from what
> generate_series() returns.

True, but how much "enforcement" do we need really?  This will be a cross
product join, which means that it can only be done as a nestloop not as a
merge or hash (there being no join key to merge or hash on).  ISTM all we
need is that the SRF be on the inside of the join, which is automatic
if it's LATERAL.

>> I think it is a very bad idea to move away from the statement that
>> a query generates a set of rows, and that no order is guaranteed
>> unless the top level has an ORDER BY clause.  How hard is it to add
>> ORDER BY 1, 2 to the above query?  Let the optimizer notice when a
>> node returns data in the needed order and skip the sort if possible.

> There's no such infrastructure for SRFS/ROWS FROM.

And in particular nothing to ORDER BY in this example.

			regards, tom lane

  71. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-02T14:25:00Z

    Andres Freund <andres@anarazel.de> writes:
> Oh, and we've previously re-added that based on
> complaints. C.f. d543170f2fdd6d9845aaf91dc0f6be7a2bf0d9e7 (and others
> IIRC).

That one wasn't about row order per se, but I agree that people *will*
bitch if we change the behavior, especially if we don't provide a way
to fix it.  ORDER BY is not a useful suggestion when there is nothing
you could order by to get the old behavior.

			regards, tom lane

  72. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-02T14:25:16Z

    On 2016-09-02 10:20:42 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-02 09:05:35 -0500, Kevin Grittner wrote:
> >>> In general, we've been skeptical about giving any guarantees about
> >>> result ordering.
> 
> > Well, it's historically how we behaved for SRFs. I'm pretty sure that
> > people will be confused if
> > SELECT generate_series(1, 10) FROM sometbl;
> > suddenly returns rows in an order that reverse from what
> > generate_series() returns.
> 
> True, but how much "enforcement" do we need really?  This will be a cross
> product join, which means that it can only be done as a nestloop not as a
> merge or hash (there being no join key to merge or hash on).  ISTM all we
> need is that the SRF be on the inside of the join, which is automatic
> if it's LATERAL.

Right. But there's nothing to force a lateral reference to be there
intrinsically. I've added a "fake" lateral reference to the ROWS FROM
RTE to the subquery, when there's none otherwise, but that's not
entirely pretty.  I'm inclined to go with that though, unless somebody
has a better idea.

Greetings,

Andres Freund

  73. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-02T14:34:54Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-02 10:20:42 -0400, Tom Lane wrote:
>> ... ISTM all we
>> need is that the SRF be on the inside of the join, which is automatic
>> if it's LATERAL.

> Right. But there's nothing to force a lateral reference to be there
> intrinsically. I've added a "fake" lateral reference to the ROWS FROM
> RTE to the subquery, when there's none otherwise, but that's not
> entirely pretty.

Hm, do you get cases like this right:

	select generate_series(1, t1.a) from t1, t2;

That would result in a lateral ref from the SRF RTE to t1, but you really
need to treat it as laterally dependent on the join of t1/t2 in order to
preserve the old semantics.  That is, you need to be laterally dependent
on the whole FROM clause regardless of which variable references appear.

			regards, tom lane

  74. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Kevin Grittner <kgrittn@gmail.com> — 2016-09-02T14:41:28Z

    On Fri, Sep 2, 2016 at 9:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Oh, and we've previously re-added that based on
>> complaints. C.f. d543170f2fdd6d9845aaf91dc0f6be7a2bf0d9e7 (and others
>> IIRC).
>
> That one wasn't about row order per se, but I agree that people *will*
> bitch if we change the behavior, especially if we don't provide a way
> to fix it.

They might also bitch if you add any overhead to put rows in a
specific order when they subsequently sort the rows into some
different order.  You might even destroy an order that would have
allowed a sort step to be skipped, so you would pay twice -- once
to put them into some "implied" order and then to sort them back
into the order they would have had without that extra effort.

> ORDER BY is not a useful suggestion when there is nothing
> you could order by to get the old behavior.

I'm apparently missing something, because I see a column with the
header "generate_series" in the result set.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  75. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-02T14:51:15Z

    Kevin Grittner <kgrittn@gmail.com> writes:
> On Fri, Sep 2, 2016 at 9:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ORDER BY is not a useful suggestion when there is nothing
>> you could order by to get the old behavior.

> I'm apparently missing something, because I see a column with the
> header "generate_series" in the result set.

You are apparently only thinking about generate_series and not any
other SRF.  Other SRFs don't necessarily produce outputs that are
in a nice sortable order.  Even for one that does, sorting by it
would destroy the existing behavior:

regression=# select *, generate_series(1,3) from int8_tbl;
        q1        |        q2         | generate_series 
------------------+-------------------+-----------------
              123 |               456 |               1
              123 |               456 |               2
              123 |               456 |               3
              123 |  4567890123456789 |               1
              123 |  4567890123456789 |               2
              123 |  4567890123456789 |               3
 4567890123456789 |               123 |               1
 4567890123456789 |               123 |               2
 4567890123456789 |               123 |               3
 4567890123456789 |  4567890123456789 |               1
 4567890123456789 |  4567890123456789 |               2
 4567890123456789 |  4567890123456789 |               3
 4567890123456789 | -4567890123456789 |               1
 4567890123456789 | -4567890123456789 |               2
 4567890123456789 | -4567890123456789 |               3
(15 rows)

Now you could argue that the ordering of the table rows
themselves is poorly defined, and you'd be right, but that
doesn't change the fact that the generate_series output
has a well-defined repeating sequence.  People might be
relying on that property.

			regards, tom lane

  76. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Kevin Grittner <kgrittn@gmail.com> — 2016-09-02T15:01:39Z

    On Fri, Sep 2, 2016 at 9:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> regression=# select *, generate_series(1,3) from int8_tbl;

I'm sure that you realize that running a query of that form twice
against a table with more than one heap page could result in rows
in a different order, even if no changes had been made to the
database (including no vacuum activity, auto- or otherwise).  If
someone reported that as a bug, what would we tell them?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  77. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-02T15:10:54Z

    Kevin Grittner <kgrittn@gmail.com> writes:
> On Fri, Sep 2, 2016 at 9:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> regression=# select *, generate_series(1,3) from int8_tbl;

> I'm sure that you realize that running a query of that form twice
> against a table with more than one heap page could result in rows
> in a different order, even if no changes had been made to the
> database (including no vacuum activity, auto- or otherwise).

You missed my point: they might complain about the generate_series
output not being in the order they expect, independently of what
the table rows are.

Also, before getting too high and mighty with users who expect
"select * from table" to produce rows in a predictable order,
you should reflect on the number of places in our regression
tests that assume exactly that ...

			regards, tom lane

  78. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-02T15:29:37Z

    On 2016-09-02 10:34:54 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-02 10:20:42 -0400, Tom Lane wrote:
> >> ... ISTM all we
> >> need is that the SRF be on the inside of the join, which is automatic
> >> if it's LATERAL.
> 
> > Right. But there's nothing to force a lateral reference to be there
> > intrinsically. I've added a "fake" lateral reference to the ROWS FROM
> > RTE to the subquery, when there's none otherwise, but that's not
> > entirely pretty.
> 
> Hm, do you get cases like this right:
> 
> 	select generate_series(1, t1.a) from t1, t2;
> 
> That would result in a lateral ref from the SRF RTE to t1, but you really
> need to treat it as laterally dependent on the join of t1/t2 in order to
> preserve the old semantics.  That is, you need to be laterally dependent
> on the whole FROM clause regardless of which variable references appear.

Yes - as the original query is moved into a subquery, the lateral
dependency I force-add simply is to the entire subquery atm (as a
wholerow var).

  79. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-02T15:31:57Z

    On 2016-09-02 09:41:28 -0500, Kevin Grittner wrote:
> On Fri, Sep 2, 2016 at 9:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> Oh, and we've previously re-added that based on
> >> complaints. C.f. d543170f2fdd6d9845aaf91dc0f6be7a2bf0d9e7 (and others
> >> IIRC).
> >
> > That one wasn't about row order per se, but I agree that people *will*
> > bitch if we change the behavior, especially if we don't provide a way
> > to fix it.
> 
> They might also bitch if you add any overhead to put rows in a
> specific order when they subsequently sort the rows into some
> different order.

Huh? It's just the order the SRFs are returning rows. If they
subsequently ORDER, there's no issue. And that doesn't have a
performance impact afaict.


> You might even destroy an order that would have
> allowed a sort step to be skipped, so you would pay twice -- once
> to put them into some "implied" order and then to sort them back
> into the order they would have had without that extra effort.

So you're arguing that you can't rely on order, but that users rely on
order?

Greetings,

Andres Freund

  80. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Kevin Grittner <kgrittn@gmail.com> — 2016-09-02T15:58:59Z

    On Fri, Sep 2, 2016 at 10:31 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-09-02 09:41:28 -0500, Kevin Grittner wrote:
>> On Fri, Sep 2, 2016 at 9:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > Andres Freund <andres@anarazel.de> writes:
>> >> Oh, and we've previously re-added that based on
>> >> complaints. C.f. d543170f2fdd6d9845aaf91dc0f6be7a2bf0d9e7 (and others
>> >> IIRC).
>> >
>> > That one wasn't about row order per se, but I agree that people *will*
>> > bitch if we change the behavior, especially if we don't provide a way
>> > to fix it.
>>
>> They might also bitch if you add any overhead to put rows in a
>> specific order when they subsequently sort the rows into some
>> different order.
>
> Huh? It's just the order the SRFs are returning rows. If they
> subsequently ORDER, there's no issue. And that doesn't have a
> performance impact afaict.

If it has no significant performance impact to maintain the
historical order, then I have no problem with doing so.  If you
burn resources putting them into historical order, that is going to
be completely wasted effort in some queries.  THAT is what I would
object to.  I'm certainly not arguing that we have any reason to go
out of our way to change the order.

>> You might even destroy an order that would have
>> allowed a sort step to be skipped, so you would pay twice -- once
>> to put them into some "implied" order and then to sort them back
>> into the order they would have had without that extra effort.
>
> So you're arguing that you can't rely on order, but that users rely on
> order?

No.  I'm arguing that we track the order coming out of different
nodes during planning, and sometimes take advantage of it to avoid
a sort which would otherwise be required.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  81. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Kevin Grittner <kgrittn@gmail.com> — 2016-09-02T16:04:20Z

    On Fri, Sep 2, 2016 at 10:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Also, before getting too high and mighty with users who expect
> "select * from table" to produce rows in a predictable order,
> you should reflect on the number of places in our regression
> tests that assume exactly that ...

An assumption that not infrequently breaks.  AFAIK, we generally
adjust the tests when that happens, rather than considering it a
bug in the code.  I never thought we did that because there was a
secret, undocumented guarantee of order, but to allow different
code paths to be tested than we would test if we always specified
an order.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  82. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Kevin Grittner <kgrittn@gmail.com> — 2016-09-02T16:08:02Z

    On Fri, Sep 2, 2016 at 10:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Kevin Grittner <kgrittn@gmail.com> writes:
>> On Fri, Sep 2, 2016 at 9:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> regression=# select *, generate_series(1,3) from int8_tbl;
>
>> I'm sure that you realize that running a query of that form twice
>> against a table with more than one heap page could result in rows
>> in a different order, even if no changes had been made to the
>> database (including no vacuum activity, auto- or otherwise).
>
> You missed my point: they might complain about the generate_series
> output not being in the order they expect, independently of what
> the table rows are.

I didn't miss it, I just never thought that anyone would care about
a secondary sort key if the primary sort key was random.  I have
trouble imagining a use-case for that.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  83. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-02T16:25:53Z

    On 2016-09-02 10:58:59 -0500, Kevin Grittner wrote:
> If it has no significant performance impact to maintain the
> historical order, then I have no problem with doing so.

It's not really a runtime issue, it's just a question of how to nicely
constraint the join order. There's no additional sorting or such.


> No.  I'm arguing that we track the order coming out of different
> nodes during planning, and sometimes take advantage of it to avoid
> a sort which would otherwise be required.

I don't think that's realistically possible with SRFs, given they're
often in some language which we have no insight on from the planner
point of view. We could possibly hack something up for SQL SRFs (that'd
be nice, but I'm doubtful it's worth it), but for everything else it
seems unrealistic.  What we could do is to add efficient
ROWS FROM (..) WITH ORDINALITY ORDER bY ordinality;
support.

Andres

  84. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-02T16:32:51Z

    Andres Freund <andres@anarazel.de> writes:
> ...  What we could do is to add efficient
> ROWS FROM (..) WITH ORDINALITY ORDER bY ordinality;
> support.

Hm?

regression=# explain select * from rows from (generate_series(1,10)) with ordinality order by ordinality;
                               QUERY PLAN                                
-------------------------------------------------------------------------
 Function Scan on generate_series  (cost=0.00..10.00 rows=1000 width=12)
(1 row)

			regards, tom lane

  85. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-02T18:16:01Z

    On 2016-09-02 14:04:24 +0530, Robert Haas wrote:
> On Sun, Aug 28, 2016 at 3:18 AM, Andres Freund <andres@anarazel.de> wrote:
> > 0003-Avoid-materializing-SRFs-in-the-FROM-list.patch
> >   To avoid performance regressions from moving SRFM_ValuePerCall SRFs to
> >   ROWS FROM, nodeFunctionscan.c needs to support not materializing
> >   output.
> >
> >   In my present patch I've *ripped out* the support for materialization
> >   in nodeFunctionscan.c entirely. That means that rescans referencing
> >   volatile functions can change their behaviour (if a function is
> >   rescanned, without having it's parameters changed), and that native
> >   backward scan support is gone.  I don't think that's actually an issue.
> 
> Can you expand on why you think those things aren't an issue?  Because
> it seems like they might be.

Backward scans are, by the planner, easily implemented by adding a
materialize node. Which will, when ordinality or multiple ROWS FROM
expressions are present, even be more runtime & memory efficient.  I
also don't think all that many people use FOR SCROLL cursors over SRFs
containing queries.

The part about rewinding is a bit more complicated. As of HEAD, a
rewound scan where some of the SRFs have to change due to parameter
inputs, but others don't, will only re-compute the ones with parameter
changes.  I don't think it's more confusing to rescan the entire input,
rather parts of it in that case.  If the entire input is re-scanned, the
planner knows how to materialize the entire scan output.

I think it'd be pretty annoying to continue to always materialize
ValuePerCall SRFs just to support that type of re-scan behaviour. We
don't really, to my knowledge, flag well whether rescans are required
atm, so we can't even easily do it conditionally.

Greetings,

Andres Freund

  86. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T14:19:14Z

    Andres Freund <andres@anarazel.de> writes:
> Attached is a significantly updated patch series (see the mail one up
> for details about what this is, I don't want to quote it in its
> entirety).

I've finally cleared my plate enough to start reviewing this patchset.

> 0001-Add-some-more-targetlist-srf-tests.patch
>   Add some test.

I think you should go ahead and push this tests-adding patch now, as it
adds documentation of the current behavior that is a good thing to have
independently of what the rest of the patchset does.  Also, I'm worried
that some of the GROUP BY tests might have machine-dependent results
(if they are implemented by hashing) so it would be good to get in a few
buildfarm cycles and let that settle out before we start changing the
answers.

I do have some trivial nitpicks about 0001:

 # rules cannot run concurrently with any test that creates a view
-test: rules psql_crosstab amutils
+test: rules psql_crosstab amutils tsrf

Although tsrf.sql doesn't currently need to create any views, it doesn't
seem like a great idea to assume that it never will.  Maybe add this
after misc_functions in the previous parallel group, instead?

+-- it's weird to GROUP BYs that increase the number of results

"it's weird to have ..."

+-- nonsensically that seems to be allowed
+UPDATE fewmore SET data = generate_series(4,9);

"nonsense that seems to be allowed..."

+-- SRFs are now allowed in RETURNING
+INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3);

s/now/not/, apparently


More to come later, but 0001 is pushable with these fixes.

			regards, tom lane

  87. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T15:29:37Z

    Andres Freund <andres@anarazel.de> writes:
> 0002-Shore-up-some-weird-corner-cases-for-targetlist-SRFs.patch
>   Forbid UPDATE ... SET foo = SRF() and ORDER BY / GROUP BY containing
>   SRFs that would change the number of returned rows.  Without the
>   latter e.g. SELECT 1 ORDER BY generate_series(1,10); returns 10 rows.

I'm on board with disallowing SRFs in UPDATE, because it produces
underdetermined and unspecified results; but the other restriction
seems 100% arbitrary.  There is no semantic difference between
	SELECT a, b FROM ... ORDER BY srf();
and
	SELECT a, b, srf() FROM ... ORDER BY 3;
except that in the first case the ordering column doesn't get returned to
the client.  I do not see why that's so awful that we should make it fail
after twenty years of allowing it.  And I certainly don't see why there
would be an implementation reason why we couldn't support it anymore
if we can still do the second one.

			regards, tom lane

  88. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T16:10:01Z

    Andres Freund <andres@anarazel.de> writes:
> 0003-Avoid-materializing-SRFs-in-the-FROM-list.patch
>   To avoid performance regressions from moving SRFM_ValuePerCall SRFs to
>   ROWS FROM, nodeFunctionscan.c needs to support not materializing
>   output.

Personally I'd put this one later, as it's a performance optimization not
part of the core patch IMO --- or is there something in the later ones
that directly depends on it?  Anyway I'm setting it aside for now.

> 0004-Allow-ROWS-FROM-to-return-functions-as-single-record.patch
>   To allow transforming SELECT record_srf(); nodeFunctionscan.c needs to
>   learn to return the result as a record.  I chose
>   ROWS FROM (record_srf() AS ()) as the syntax for that. It doesn't
>   necessarily have to be SQL exposed, but it does make testing easier.

The proposed commit message is wrong, as it claims aclexplode()
demonstrates the problem which it doesn't --- we get the column set
from the function's declared OUT parameters.

I can't say that I like the proposed syntax much.  What about leaving out
any syntax changes, and simply saying that "if the function returns record
and hasn't got OUT parameters, then return its result as an unexpanded
record"?  That might not take much more than removing the error check ;-)

A possible objection is that then you could not get the no-expansion
behavior for functions that return named composite types or have OUT
parameters that effectively give them known composite types.  From a
semantic standpoint we could fix that by saying "just cast the result to
record", ie ROWS FROM (aclexplode('whatever')::record) would give the
no-expansion behavior.  I'm not sure if there might be any implementation
glitches in the way of doing it like that.  Also there seems to be some
syntactic issue with it ... actually, the current behavior there is just
weird:

regression=# select * from rows from (aclexplode('{=r/postgres}')::record); 
ERROR:  syntax error at or near "::"
LINE 1: ...lect * from rows from (aclexplode('{=r/postgres}')::record);
                                                             ^
regression=# select * from rows from (cast(aclexplode('{=r/postgres}') as record));
 grantor | grantee | privilege_type | is_grantable 
---------+---------+----------------+--------------
      10 |       0 | SELECT         | f
(1 row)

I was not aware that there was *anyplace* in the grammar where :: and CAST
behaved differently, and I'm not very pleased to find this one.

I haven't looked at the code, as there probably isn't much point in
reviewing in any detail till we choose the syntax.

			regards, tom lane

  89. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T16:14:47Z

    On 2016-09-12 10:19:14 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> 
> > 0001-Add-some-more-targetlist-srf-tests.patch
> >   Add some test.
> 
> I think you should go ahead and push this tests-adding patch now, as it
> adds documentation of the current behavior that is a good thing to have
> independently of what the rest of the patchset does.  Also, I'm worried
> that some of the GROUP BY tests might have machine-dependent results
> (if they are implemented by hashing) so it would be good to get in a few
> buildfarm cycles and let that settle out before we start changing the
> answers.

Generally a sound plan - I started to noticeably expand it though,
there's some important edge cases it didn't cover.


> Although tsrf.sql doesn't currently need to create any views, it doesn't
> seem like a great idea to assume that it never will.  Maybe add this
> after misc_functions in the previous parallel group, instead?

WFM


> +-- it's weird to GROUP BYs that increase the number of results
> 
> "it's weird to have ..."
> 
> +-- nonsensically that seems to be allowed
> +UPDATE fewmore SET data = generate_series(4,9);
> 
> "nonsense that seems to be allowed..."
> 
> +-- SRFs are now allowed in RETURNING
> +INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3);
> 
> s/now/not/, apparently

Err, yes. Will update.


Greetings,

Andres Freund

  90. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T16:16:59Z

    On 2016-09-12 11:29:37 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > 0002-Shore-up-some-weird-corner-cases-for-targetlist-SRFs.patch
> >   Forbid UPDATE ... SET foo = SRF() and ORDER BY / GROUP BY containing
> >   SRFs that would change the number of returned rows.  Without the
> >   latter e.g. SELECT 1 ORDER BY generate_series(1,10); returns 10 rows.
> 
> I'm on board with disallowing SRFs in UPDATE, because it produces
> underdetermined and unspecified results; but the other restriction
> seems 100% arbitrary.  There is no semantic difference between
> 	SELECT a, b FROM ... ORDER BY srf();
> and
> 	SELECT a, b, srf() FROM ... ORDER BY 3;
> except that in the first case the ordering column doesn't get returned to
> the client.  I do not see why that's so awful that we should make it fail
> after twenty years of allowing it.

I do think it's awful that an ORDER BY / GROUP BY changes the number of
rows processed. This should never have been allowed. You just need a
little typo somewhere that makes the targetlist entry not match the
ORDER/GROUP BY anymore and your results will differ in weird ways -
rather hard to debug in the GROUP BY case.


> And I certainly don't see why there
> would be an implementation reason why we couldn't support it anymore
> if we can still do the second one.

There's nothing requiring this here, indeed.


Andres

  91. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T16:25:34Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 11:29:37 -0400, Tom Lane wrote:
>> I'm on board with disallowing SRFs in UPDATE, because it produces
>> underdetermined and unspecified results; but the other restriction
>> seems 100% arbitrary.  There is no semantic difference between
>> SELECT a, b FROM ... ORDER BY srf();
>> and
>> SELECT a, b, srf() FROM ... ORDER BY 3;
>> except that in the first case the ordering column doesn't get returned to
>> the client.  I do not see why that's so awful that we should make it fail
>> after twenty years of allowing it.

> I do think it's awful that an ORDER BY / GROUP BY changes the number of
> rows processed. This should never have been allowed.

Meh.  That's just an opinion, and it's a bit late to be making such
changes.  I think the general consensus of the previous discussion was
that we would preserve existing tSRF behavior as far as it was reasonably
practical to do so, with the exception that there's wide agreement that
the least-common-multiple rule for number of rows emitted is bad.  I do
not think you're going to get anywhere near that level of agreement that
a SRF appearing only in ORDER BY is bad.

			regards, tom lane

  92. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T16:54:03Z

    Hi,

On 2016-09-12 12:10:01 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > 0003-Avoid-materializing-SRFs-in-the-FROM-list.patch
> >   To avoid performance regressions from moving SRFM_ValuePerCall SRFs to
> >   ROWS FROM, nodeFunctionscan.c needs to support not materializing
> >   output.
> 
> Personally I'd put this one later, as it's a performance optimization not
> part of the core patch IMO --- or is there something in the later ones
> that directly depends on it?  Anyway I'm setting it aside for now.

Not strongly dependant. But the ROWS FROM stuff touches a lot of the
same code. And I wanted to implement this before ripping out the current
implementation, to allow for meaningful performance tests.

> > 0004-Allow-ROWS-FROM-to-return-functions-as-single-record.patch
> >   To allow transforming SELECT record_srf(); nodeFunctionscan.c needs to
> >   learn to return the result as a record.  I chose
> >   ROWS FROM (record_srf() AS ()) as the syntax for that. It doesn't
> >   necessarily have to be SQL exposed, but it does make testing easier.
> 
> The proposed commit message is wrong, as it claims aclexplode()
> demonstrates the problem which it doesn't --- we get the column set
> from the function's declared OUT parameters.

Oops. I'd probably tested with some self defined function and was
looking for an example...


> I can't say that I like the proposed syntax much.

Me neither. But I haven't really found a better approach.  It seems
kinda consistent to have ROWS FROM (... AS ()) change the picked out
columns to 0, and just return the whole thing.


> What about leaving out
> any syntax changes, and simply saying that "if the function returns record
> and hasn't got OUT parameters, then return its result as an unexpanded
> record"?  That might not take much more than removing the error check ;-)

Having the ability to do this for non-record returning functions turned
out to be quite convenient. Otherwise we need to create ROW()
expressions for composite returning functions, which is neither cheap,
nor fun..

As you say, that might be doable with some form of casting, but,
ugh. I'm actually kind of surprised that even works. The function call
that nodeFunctionscan.c sees, isn't a function call, much less a set
returning one. Which means this hits the direct_function_call == false
path in ExecMakeTableFunctionResult(). If it didn't, we'd have hit
		/* We don't allow sets in the arguments of the table function */
		if (argDone != ExprSingleResult)
			ereport(ERROR,
					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
					 errmsg("set-valued function called in context that cannot accept a set")));
therein. Which you'd hit e.g. with
SELECT * FROM ROWS FROM (int4mul(generate_series(1, 2), 3));

Thus this actually relies on the SRF code path in execQual.c;
the thing we want to rip out...

> A possible objection is that then you could not get the no-expansion
> behavior for functions that return named composite types or have OUT
> parameters that effectively give them known composite types.  From a
> semantic standpoint we could fix that by saying "just cast the result to
> record", ie ROWS FROM (aclexplode('whatever')::record) would give the
> no-expansion behavior.  I'm not sure if there might be any implementation
> glitches in the way of doing it like that.

See above.  Personally I think just using explicit syntax is cleaner,
but I don't feel like arguing about this a whole lot.


- Andres

  93. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T17:26:20Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 12:10:01 -0400, Tom Lane wrote:
>> I can't say that I like the proposed syntax much.

> Me neither. But I haven't really found a better approach.  It seems
> kinda consistent to have ROWS FROM (... AS ()) change the picked out
> columns to 0, and just return the whole thing.

I just remembered that we allow zero-column composite types, which
makes this proposal formally ambiguous.  So we really need a different
syntax.  I'm not especially in love with the cast-to-record idea, but
it does dodge that problem.

Stepping back a little bit ... way back at the start of this thread
you muttered about possibly implementing tSRFs as a special pipeline
node type, a la Result.  That would have the same benefits in terms
of being able to take SRF support out of the main execQual code paths.
I, and I think some other people, felt that the LATERAL approach would
be a cleaner answer --- but now that we're seeing some of the messy
details required to make the LATERAL way work, I'm beginning to have
second thoughts.  I wonder if we should do at least a POC implementation
of the other way to get a better fix on which way is really cleaner.

Also, one of the points that's come up repeatedly in these discussions
is the way that the parser's implementation of *-expansion sucks for
composite-returning functions.  That is, if you write
	SELECT (foo(...)).* FROM ...
you get
	SELECT (foo(...)).col1, (foo(...)).col2, ... FROM ...
so that the function is executed N times not once.  We had discussed
fixing that for setof-composite-returning functions by folding multiple
identical SRF calls into a single LATERAL entry, but that doesn't
directly fix the problem for non-SRF composite functions.  Also the
whole idea of having the planner undo the parser's damage in this way
is kinda grotty, not least because we can't safely combine multiple
calls of volatile functions, so it only works for not-volatile ones.

That line of thought leads to the idea that if we could have the *parser*
do the transformation to LATERAL form, we could avoid breaking a
composite-returning function call into multiple copies in the first place.
I had said that I didn't think we wanted this transformation done in the
parser, but maybe this is a sufficient reason to do so.

If we think in terms of pipeline evaluation nodes rather than LATERAL,
we could implement the above by having the parser emit multiple levels
of SELECT some-expressions FROM (SELECT some-expressions FROM ...),
with SRFs being rigidly separated into their own evaluation levels.

I'm not certain that any of these ideas are worth the electrons they're
written on, but I do think we ought to consider alternatives and not
just push forward with committing a first-draft implementation.

			regards, tom lane

  94. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T17:38:15Z

    Hi,


On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 12:10:01 -0400, Tom Lane wrote:
> >> I can't say that I like the proposed syntax much.
> 
> > Me neither. But I haven't really found a better approach.  It seems
> > kinda consistent to have ROWS FROM (... AS ()) change the picked out
> > columns to 0, and just return the whole thing.
> 
> I just remembered that we allow zero-column composite types, which
> makes this proposal formally ambiguous.

Well, we errored out in the grammar for AS () so far... We might want to
fix that independently.


> Stepping back a little bit ... way back at the start of this thread
> you muttered about possibly implementing tSRFs as a special pipeline
> node type, a la Result.  That would have the same benefits in terms
> of being able to take SRF support out of the main execQual code paths.
> I, and I think some other people, felt that the LATERAL approach would
> be a cleaner answer --- but now that we're seeing some of the messy
> details required to make the LATERAL way work, I'm beginning to have
> second thoughts.  I wonder if we should do at least a POC implementation
> of the other way to get a better fix on which way is really cleaner.

I'm not particularly in love in restarting with a different approach. I
think fixing the ROWS FROM expansion is the only really painful bit, and
that seems like it's independently beneficial to allow for suppression
of expansion there.  I'm working on this to actually be finally able to
get some stuff from the "faster executor" thread in a committable
shape,...  The other stuff like making SELECT * FROM func; not
materialize also seems independently useful; it's something people have
complained about repeatedly over the years.


I actually had started to work on a Result style approach, and I don't
think it turned out that nice. But I didn't complete it, so I might just
be wrong.


> Also, one of the points that's come up repeatedly in these discussions
> is the way that the parser's implementation of *-expansion sucks for
> composite-returning functions.  That is, if you write
> 	SELECT (foo(...)).* FROM ...
> you get
> 	SELECT (foo(...)).col1, (foo(...)).col2, ... FROM ...
> so that the function is executed N times not once.  We had discussed
> fixing that for setof-composite-returning functions by folding multiple
> identical SRF calls into a single LATERAL entry, but that doesn't
> directly fix the problem for non-SRF composite functions.  Also the
> whole idea of having the planner undo the parser's damage in this way
> is kinda grotty, not least because we can't safely combine multiple
> calls of volatile functions, so it only works for not-volatile ones.


> That line of thought leads to the idea that if we could have the *parser*
> do the transformation to LATERAL form, we could avoid breaking a
> composite-returning function call into multiple copies in the first place.
> I had said that I didn't think we wanted this transformation done in the
> parser, but maybe this is a sufficient reason to do so.

I still don't like doing all this is in the parser. It'd just trigger
complaints of users that we're changing their query structure, and we'd
have to solve a good bit of the same problems we have to solve here.

If we really want to reduce the expansion cost - and to me that's a
largely independent issue from this - it seems better to have the parser
emit some structure that's easily recognized at plan time, rather than
have the praser do all the work.


Andres

  95. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T17:44:43Z

    On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 12:10:01 -0400, Tom Lane wrote:
> >> I can't say that I like the proposed syntax much.
> 
> > Me neither. But I haven't really found a better approach.  It seems
> > kinda consistent to have ROWS FROM (... AS ()) change the picked out
> > columns to 0, and just return the whole thing.
> 
> I just remembered that we allow zero-column composite types, which
> makes this proposal formally ambiguous.  So we really need a different
> syntax.  I'm not especially in love with the cast-to-record idea, but
> it does dodge that problem.

I kind of like ROWS FROM (... AS VALUE), that seems to confer the
meaning quite well. As VALUE isn't a reserved keyword, that'd afaik only
really work inside ROWS FROM() where AS is required.

  96. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T17:48:05Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 12:10:01 -0400, Tom Lane wrote:
>>>> I can't say that I like the proposed syntax much.

>>> Me neither. But I haven't really found a better approach.  It seems
>>> kinda consistent to have ROWS FROM (... AS ()) change the picked out
>>> columns to 0, and just return the whole thing.

>> I just remembered that we allow zero-column composite types, which
>> makes this proposal formally ambiguous.  So we really need a different
>> syntax.  I'm not especially in love with the cast-to-record idea, but
>> it does dodge that problem.

> I kind of like ROWS FROM (... AS VALUE), that seems to confer the
> meaning quite well. As VALUE isn't a reserved keyword, that'd afaik only
> really work inside ROWS FROM() where AS is required.

Hm, wouldn't ... AS RECORD convey the meaning better?

(Although once you look at it that way, it's just a cast spelled in
an idiosyncratic fashion.)

			regards, tom lane

  97. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T17:58:11Z

    On 2016-09-12 13:48:05 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 12:10:01 -0400, Tom Lane wrote:
> >>>> I can't say that I like the proposed syntax much.
>
> >>> Me neither. But I haven't really found a better approach.  It seems
> >>> kinda consistent to have ROWS FROM (... AS ()) change the picked out
> >>> columns to 0, and just return the whole thing.
>
> >> I just remembered that we allow zero-column composite types, which
> >> makes this proposal formally ambiguous.  So we really need a different
> >> syntax.  I'm not especially in love with the cast-to-record idea, but
> >> it does dodge that problem.
>
> > I kind of like ROWS FROM (... AS VALUE), that seems to confer the
> > meaning quite well. As VALUE isn't a reserved keyword, that'd afaik only
> > really work inside ROWS FROM() where AS is required.
>
> Hm, wouldn't ... AS RECORD convey the meaning better?

I was kind of envisioning AS VALUE to work for composite types without
removing their original type (possibly even for TYPEFUNC_SCALAR
ones). That, for one, makes the SRF to ROWS FROM conversion easier, and
for another seems generally useful. composites keeping their type with
AS RECORD seems a bit confusing.  There's also the issue that VALUE is
already a keyword, record not...

> (Although once you look at it that way, it's just a cast spelled in
> an idiosyncratic fashion.)

Well, not quite, by virtue of keeping the original type around. After a
record cast you likely couldn't directly access the columns anymore,
even if it were a known composite type, right?

Andres

  98. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T18:05:33Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 13:48:05 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> I kind of like ROWS FROM (... AS VALUE), that seems to confer the
>>> meaning quite well. As VALUE isn't a reserved keyword, that'd afaik only
>>> really work inside ROWS FROM() where AS is required.

>> Hm, wouldn't ... AS RECORD convey the meaning better?

> I was kind of envisioning AS VALUE to work for composite types without
> removing their original type (possibly even for TYPEFUNC_SCALAR
> ones).

Maybe.  A problem with any of these proposals though is that there's no
place to put a column alias.  Yeah, you can stick it on outside the ROWS
FROM, but it seems a bit non-orthogonal to have to do it that way when
you can do it inside the ROWS FROM when adding a coldeflist.

Maybe we could do it like

	ROWS FROM (func(...) AS alias)

where the difference from a coldeflist is that there's no parenthesized
list of names/types.  It's a bit weird that adding an alias makes for
a semantic not just naming difference, but it's no weirder than these
other ideas.

>> (Although once you look at it that way, it's just a cast spelled in
>> an idiosyncratic fashion.)

> Well, not quite, by virtue of keeping the original type around. After a
> record cast you likely couldn't directly access the columns anymore,
> even if it were a known composite type, right?

Same is true for any of these syntax proposals, no?  So far as the rest of
the query is concerned, the function output is going to be an anonymous
record type.

			regards, tom lane

  99. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T18:18:11Z

    On 2016-09-12 14:05:33 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 13:48:05 -0400, Tom Lane wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> >>> I kind of like ROWS FROM (... AS VALUE), that seems to confer the
> >>> meaning quite well. As VALUE isn't a reserved keyword, that'd afaik only
> >>> really work inside ROWS FROM() where AS is required.
> 
> >> Hm, wouldn't ... AS RECORD convey the meaning better?
> 
> > I was kind of envisioning AS VALUE to work for composite types without
> > removing their original type (possibly even for TYPEFUNC_SCALAR
> > ones).
> 
> Maybe.  A problem with any of these proposals though is that there's no
> place to put a column alias.  Yeah, you can stick it on outside the ROWS
> FROM, but it seems a bit non-orthogonal to have to do it that way when
> you can do it inside the ROWS FROM when adding a coldeflist.

I don't necessarily see that as a problem. The coldeflists inside ROWS
FROM() already don't allow assigning aliases for !record/composite
types, and they require specifying types.


> >> (Although once you look at it that way, it's just a cast spelled in
> >> an idiosyncratic fashion.)
> 
> > Well, not quite, by virtue of keeping the original type around. After a
> > record cast you likely couldn't directly access the columns anymore,
> > even if it were a known composite type, right?
> 
> Same is true for any of these syntax proposals, no?  So far as the rest of
> the query is concerned, the function output is going to be an anonymous
> record type.

Not for composite types, no. As implemented ROWS FROM (.. AS()) does:
CREATE OR REPLACE FUNCTION get_pg_class() RETURNS SETOF pg_class LANGUAGE sql AS $$SELECT * FROM pg_class;$$;
SELECT DISTINCT pg_typeof(f) FROM ROWS FROM (get_pg_class() AS ()) f;
┌───────────┐
│ pg_typeof │
├───────────┤
│ pg_class  │
└───────────┘
(1 row)
SELECT (f).relname FROM ROWS FROM (get_pg_class() AS ()) f LIMIT 1;
┌────────────────┐
│    relname     │
├────────────────┤
│ pg_toast_77994 │
└────────────────┘
(1 row)
which seems sensible to me.

  100. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T21:36:07Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
>> Stepping back a little bit ... way back at the start of this thread
>> you muttered about possibly implementing tSRFs as a special pipeline
>> node type, a la Result.  That would have the same benefits in terms
>> of being able to take SRF support out of the main execQual code paths.
>> I, and I think some other people, felt that the LATERAL approach would
>> be a cleaner answer --- but now that we're seeing some of the messy
>> details required to make the LATERAL way work, I'm beginning to have
>> second thoughts.  I wonder if we should do at least a POC implementation
>> of the other way to get a better fix on which way is really cleaner.

> I'm not particularly in love in restarting with a different approach. I
> think fixing the ROWS FROM expansion is the only really painful bit, and
> that seems like it's independently beneficial to allow for suppression
> of expansion there.

Um, I dunno.  You've added half a thousand lines of not-highly-readable-
nor-extensively-commented code to the planner; that certainly reaches *my*
threshold of pain.  I'm also growing rather concerned that the LATERAL
approach is going to lock us into some unremovable incompatibilities
no matter how much we might regret that later (and in view of how quickly
I got my wrist slapped in <001201d18524$f84c4580$e8e4d080$@pcorp.us>,
I am afraid there may be more pushback awaiting us than we think).
If we go with a Result-like tSRF evaluation node, then whether we change
semantics or not becomes mostly a matter of what that node does.  It could
become basically a wrapper around the existing ExecTargetList() logic if
we needed to provide backwards-compatible behavior.

> I actually had started to work on a Result style approach, and I don't
> think it turned out that nice. But I didn't complete it, so I might just
> be wrong.

It's also possible that it's easier now in the post-pathification code
base than it was before.  After contemplating my navel for awhile, it
seems like the core of the planner code for a Result-like approach would
be something very close to make_group_input_target(), plus a thing like
pull_var_clause() that extracts SRFs rather than Vars.  Those two
functions, including their lengthy comments, weigh in at ~250 lines.
Admittedly there'd be some boilerplate on top of that, if we invent a
new plan node type rather than extending Result, but not all that much.

If you like, I'll have a go at drafting a patch in that style.  Do you
happen to still have the executor side of what you did, so I don't have
to reinvent that?

			regards, tom lane

  101. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T22:07:05Z

    On 2016-09-12 17:36:07 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
> >> Stepping back a little bit ... way back at the start of this thread
> >> you muttered about possibly implementing tSRFs as a special pipeline
> >> node type, a la Result.  That would have the same benefits in terms
> >> of being able to take SRF support out of the main execQual code paths.
> >> I, and I think some other people, felt that the LATERAL approach would
> >> be a cleaner answer --- but now that we're seeing some of the messy
> >> details required to make the LATERAL way work, I'm beginning to have
> >> second thoughts.  I wonder if we should do at least a POC implementation
> >> of the other way to get a better fix on which way is really cleaner.
> 
> > I'm not particularly in love in restarting with a different approach. I
> > think fixing the ROWS FROM expansion is the only really painful bit, and
> > that seems like it's independently beneficial to allow for suppression
> > of expansion there.
> 
> Um, I dunno.  You've added half a thousand lines of not-highly-readable-
> nor-extensively-commented code to the planner; that certainly reaches *my*
> threshold of pain.

Well, I certainly plan (and started to) make that code easier to
understand, and better commented.  It also removes ~1400 LOC of not easy
to understand code... A good chunk of that'd would also be removed with
a Result style approach, but far from all.


> I'm also growing rather concerned that the LATERAL
> approach is going to lock us into some unremovable incompatibilities
> no matter how much we might regret that later (and in view of how quickly
> I got my wrist slapped in <001201d18524$f84c4580$e8e4d080$@pcorp.us>,
> I am afraid there may be more pushback awaiting us than we think).

I don't think it'd be all that hard to add something like the current
LCM behaviour into nodeFunctionscan.c if we really wanted. But I think
it'll be better to just say no here.


> If we go with a Result-like tSRF evaluation node, then whether we change
> semantics or not becomes mostly a matter of what that node does.  It could
> become basically a wrapper around the existing ExecTargetList() logic if
> we needed to provide backwards-compatible behavior.

As you previously objected: If we keep ExecTargetList() style logic, we
need to keep most of execQual.c's handling of ExprMultipleResult et al,
and that's going to prevent the stuff I want to work on. Because
handling ExprMultipleResult in all these places is a serious issue
WRT making expression evaluation faster.  If we find a good answer to
that, I'd be more open to pursuing this approach.


> > I actually had started to work on a Result style approach, and I don't
> > think it turned out that nice. But I didn't complete it, so I might just
> > be wrong.
> 
> It's also possible that it's easier now in the post-pathification code
> base than it was before.  After contemplating my navel for awhile, it
> seems like the core of the planner code for a Result-like approach would
> be something very close to make_group_input_target(), plus a thing like
> pull_var_clause() that extracts SRFs rather than Vars.  Those two
> functions, including their lengthy comments, weigh in at ~250 lines.
> Admittedly there'd be some boilerplate on top of that, if we invent a
> new plan node type rather than extending Result, but not all that much.

That's pretty much what I did (or rather started to do), yes.  I had
something that was called from grouping_planner() that added the new
node ontop of the original set of paths, after grouping or after
distinct, depending on where SRFs were referenced.  The biggest benefit
I saw with that is that there's no need to push things into a subquery,
and that the ordering is a lot more explicit.


> If you like, I'll have a go at drafting a patch in that style.  Do you
> happen to still have the executor side of what you did, so I don't have
> to reinvent that?

The executor side is actually what I found harder here. Either we end up
keeping most of ExecQual's handling, or we reinvent a good deal of
separate logic.

Greetings,

Andres Freund

  102. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T22:35:03Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 17:36:07 -0400, Tom Lane wrote:
>> Um, I dunno.  You've added half a thousand lines of not-highly-readable-
>> nor-extensively-commented code to the planner; that certainly reaches *my*
>> threshold of pain.

> Well, I certainly plan (and started to) make that code easier to
> understand, and better commented.  It also removes ~1400 LOC of not easy
> to understand code... A good chunk of that'd would also be removed with
> a Result style approach, but far from all.

Hm, I've not studied 0006 yet, but surely that's executor code that would
go away with *any* approach that takes away the need for generic execQual
to support SRFs?  I don't see that it counts while discussing which way
we take to reach that point.

>> I'm also growing rather concerned that the LATERAL
>> approach is going to lock us into some unremovable incompatibilities
>> no matter how much we might regret that later (and in view of how quickly
>> I got my wrist slapped in <001201d18524$f84c4580$e8e4d080$@pcorp.us>,
>> I am afraid there may be more pushback awaiting us than we think).

> I don't think it'd be all that hard to add something like the current
> LCM behaviour into nodeFunctionscan.c if we really wanted. But I think
> it'll be better to just say no here.

"Just say no" soon translates to memes about "disasters like the removal
of implicit casting" (which in fact is not what 8.3 did, but I've grown
pretty damn tired of the amount of bitching that that cleanup did and
still does provoke).  In any case, it feels like the LATERAL approach is
locking us into more and subtler incompatibilities than just that one.

>> If we go with a Result-like tSRF evaluation node, then whether we change
>> semantics or not becomes mostly a matter of what that node does.  It could
>> become basically a wrapper around the existing ExecTargetList() logic if
>> we needed to provide backwards-compatible behavior.

> As you previously objected: If we keep ExecTargetList() style logic, we
> need to keep most of execQual.c's handling of ExprMultipleResult et al,
> and that's going to prevent the stuff I want to work on.

You're inventing objections.  It won't require that any more than the
LATERAL approach does; it's basically the same code as whatever
nodeFunctionscan is going to do, but packaged as a pipeline eval node
rather than a base scan node.  Or to be clearer: what I'm suggesting it
would contain is ExecTargetList's logic about restarting individual SRFs.
That wouldn't propagate into execQual because we would only allow SRFs at
the top level of the node's tlist, just like nodeFunctionscan does.
ExecMakeTableFunctionResult doesn't require the generic execQual code
to support SRFs today, and it still wouldn't.

In larger terms: the whole point here is to fish SRF calls up to the top
level of the tlist of whatever node is executing them, where they can be
special-cased by that node.  Their SRF-free argument expressions would be
evaluated by generic execQual.  AFAICS this goes through in the same way
from the executor's viewpoint whether we use LATERAL as the query
restructuring method or a SRF-capable variant of Result.  But it's now
looking to me like the latter would be a lot simpler from the point of
view of planner complexity, and in particular from the point of view of
proving correctness (equivalence of the query transformation).

			regards, tom lane

  103. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T23:11:44Z

    Hi,

On 2016-09-12 18:35:03 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I don't think it'd be all that hard to add something like the current
> > LCM behaviour into nodeFunctionscan.c if we really wanted. But I think
> > it'll be better to just say no here.
> 
> "Just say no" soon translates to memes about "disasters like the removal
> of implicit casting" (which in fact is not what 8.3 did, but I've grown
> pretty damn tired of the amount of bitching that that cleanup did and
> still does provoke).  In any case, it feels like the LATERAL approach is
> locking us into more and subtler incompatibilities than just that one.

I can't see those being equivalent impact-wise. Note that the person
bitching most loudly about the "implicit casting" thing (Merlin Moncure)
is voting to remove the LCM behaviour ;)

I think we'll continue to get more bitching about the confusing LCM
behaviour than complaints the backward compat break would generate.


> >> If we go with a Result-like tSRF evaluation node, then whether we change
> >> semantics or not becomes mostly a matter of what that node does.  It could
> >> become basically a wrapper around the existing ExecTargetList() logic if
> >> we needed to provide backwards-compatible behavior.
> 
> > As you previously objected: If we keep ExecTargetList() style logic, we
> > need to keep most of execQual.c's handling of ExprMultipleResult et al,
> > and that's going to prevent the stuff I want to work on.
> 
> You're inventing objections.

Heh, it's actually your own objection ;)

http://archives.postgresql.org/message-id/32673.1464023429%40sss.pgh.pa.us


> It won't require that any more than the
> LATERAL approach does; it's basically the same code as whatever
> nodeFunctionscan is going to do, but packaged as a pipeline eval node
> rather than a base scan node.  Or to be clearer: what I'm suggesting it
> would contain is ExecTargetList's logic about restarting individual SRFs.
> That wouldn't propagate into execQual because we would only allow SRFs at
> the top level of the node's tlist, just like nodeFunctionscan does.
> ExecMakeTableFunctionResult doesn't require the generic execQual code
> to support SRFs today, and it still wouldn't.

(it accidentally does (see your cast example), but that that's besides
your point)

That might work.  It gets somewhat nasty though because you also need to
handle, SRF arguments to SRFs. And those again can contain nearly
arbitrary expressions inbetween. With the ROWS FROM approach that can be
fairly easily handled via LATERAL.  I guess what we could do here is to
use one pipeline node to evaluate all the argument SRFs, and then
another for the outer expression. Where the outer node would evaluate
the SRF arguments using normal expression evaluation, with the inner SRF
output replaced by a var.

I wonder how much duplication we'd end up between nodeFunctionscan.c and
nodeSRF (or whatever). We'd need the latter node to support ValuePerCall
in an non-materializing fashion as well.  Could we combine them somehow?


> In larger terms: the whole point here is to fish SRF calls up to the
> top level of the tlist of whatever node is executing them, where they
> can be special-cased by that node.  Their SRF-free argument
> expressions would be evaluated by generic execQual.  AFAICS this goes
> through in the same way from the executor's viewpoint whether we use
> LATERAL as the query restructuring method or a SRF-capable variant of
> Result.  But it's now looking to me like the latter would be a lot
> simpler from the point of view of planner complexity, and in
> particular from the point of view of proving correctness (equivalence
> of the query transformation).

It's nicer not to introduce subqueries from a two angles from my pov:
1) EXPLAINs will look more like the original query
2) The evaluation order of the non-srf part of the query, and the query
   itself, will be easier. That's by the thing I'm least happy with the
   LATERAL approach.

I don't think the code for adding these intermediate SRF evaluating
nodes will be noticeably simpler than what's in my prototype. We'll
still have to do the whole conversion recursively, we'll still need
complexity of figuring out whether to put those SRFs evaluations
before/after group by, order by, distinct on and window functions.

Greetings,

Andres Freund

  104. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-12T23:35:22Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 18:35:03 -0400, Tom Lane wrote:
>> You're inventing objections.

> Heh, it's actually your own objection ;)
> http://archives.postgresql.org/message-id/32673.1464023429%40sss.pgh.pa.us

I'm changing my opinion in the light of unfavorable evidence.  Is that
wrong?

>> It won't require that any more than the
>> LATERAL approach does; it's basically the same code as whatever
>> nodeFunctionscan is going to do, but packaged as a pipeline eval node
>> rather than a base scan node.

> That might work.  It gets somewhat nasty though because you also need to
> handle, SRF arguments to SRFs. And those again can contain nearly
> arbitrary expressions inbetween. With the ROWS FROM approach that can be
> fairly easily handled via LATERAL.  I guess what we could do here is to
> use one pipeline node to evaluate all the argument SRFs, and then
> another for the outer expression. Where the outer node would evaluate
> the SRF arguments using normal expression evaluation, with the inner SRF
> output replaced by a var.

Right.  Nested SRFs translate to multiple ROWS-FROM RTEs with lateral
references in the one approach, and nested Result-thingies in the other.
It's pretty much the same thing mutatis mutandis, but I think it will
likely be a lot easier to get there from here with the Result-based
approach --- for example, we don't have to worry about forcing lateral
join order, and the ordering constraints vis-a-vis GROUP BY etc won't take
any great effort either.  Anyway I think it is worth trying.

> I wonder how much duplication we'd end up between nodeFunctionscan.c and
> nodeSRF (or whatever). We'd need the latter node to support ValuePerCall
> in an non-materializing fashion as well.  Could we combine them somehow?

Yeah, I was wondering that too.  I doubt that we want to make one node
type do both things --- the fact that Result comes in two flavors with
different semantics (with or without an input node) isn't very nice IMO,
and this would be almost that identical case.  But maybe they could share
some code at the level of ExecMakeTableFunctionResult.  (I've not looked
at your executor changes yet, not sure how much of that still exists.)

> I don't think the code for adding these intermediate SRF evaluating
> nodes will be noticeably simpler than what's in my prototype. We'll
> still have to do the whole conversion recursively, we'll still need
> complexity of figuring out whether to put those SRFs evaluations
> before/after group by, order by, distinct on and window functions.

I think it will slot into the code that's already there rather more
easily than what you've done, because we already *have* code that makes
decisions in that form.  We just need to teach it to break down what
it now thinks of as a single projection step into N+1 steps when there
are N levels of nested SRF present.  Anyway I'll draft a prototype and
then we can compare.

			regards, tom lane

  105. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T23:46:30Z

    On 2016-09-12 19:35:22 -0400, Tom Lane wrote:
> >> You're inventing objections.
> 
> > Heh, it's actually your own objection ;)
> > http://archives.postgresql.org/message-id/32673.1464023429%40sss.pgh.pa.us
> 
> I'm changing my opinion in the light of unfavorable evidence.  Is that
> wrong?

Nah, not at all.  I was just referring to "inventing".


> > I wonder how much duplication we'd end up between nodeFunctionscan.c and
> > nodeSRF (or whatever). We'd need the latter node to support ValuePerCall
> > in an non-materializing fashion as well.  Could we combine them somehow?
> 
> Yeah, I was wondering that too.  I doubt that we want to make one node
> type do both things --- the fact that Result comes in two flavors with
> different semantics (with or without an input node) isn't very nice IMO,
> and this would be almost that identical case.

It might not, agreed. That imo has to be taken into acount calculating
the code costs - although the executor stuff usually is pretty boring in
comparison.


> But maybe they could share
> some code at the level of ExecMakeTableFunctionResult.  (I've not looked
> at your executor changes yet, not sure how much of that still exists.)

I'd split ExecMakeTableFunctionResult up, to allow for a percall mode,
and that seems like it'd still be needed to avoid performance
regressions.

It'd be an awfully large amount of code those two nodes would
duplicate. Excepting different rescan logic and ORDINALITY support,
nearly all the nodeFunctionscan.c code would be needed in both nodes.

> Anyway I'll draft a prototype and then we can compare.

Ok, cool.


Andres

  106. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T23:56:32Z

    On 2016-09-12 09:14:47 -0700, Andres Freund wrote:
> On 2016-09-12 10:19:14 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > 
> > > 0001-Add-some-more-targetlist-srf-tests.patch
> > >   Add some test.
> > 
> > I think you should go ahead and push this tests-adding patch now, as it
> > adds documentation of the current behavior that is a good thing to have
> > independently of what the rest of the patchset does.  Also, I'm worried
> > that some of the GROUP BY tests might have machine-dependent results
> > (if they are implemented by hashing) so it would be good to get in a few
> > buildfarm cycles and let that settle out before we start changing the
> > answers.
> 
> Generally a sound plan - I started to noticeably expand it though,
> there's some important edge cases it didn't cover.

Attached is a noticeably expanded set of tests, with fixes for the stuff
you'd found.  I plan to push that soon-ish.  Independent of the approach
we'll be choosing, increased coverage seems quite useful.

Andres

  107. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-12T23:58:06Z

    On 2016-09-12 16:56:32 -0700, Andres Freund wrote:
> On 2016-09-12 09:14:47 -0700, Andres Freund wrote:
> > On 2016-09-12 10:19:14 -0400, Tom Lane wrote:
> > > Andres Freund <andres@anarazel.de> writes:
> > > 
> > > > 0001-Add-some-more-targetlist-srf-tests.patch
> > > >   Add some test.
> > > 
> > > I think you should go ahead and push this tests-adding patch now, as it
> > > adds documentation of the current behavior that is a good thing to have
> > > independently of what the rest of the patchset does.  Also, I'm worried
> > > that some of the GROUP BY tests might have machine-dependent results
> > > (if they are implemented by hashing) so it would be good to get in a few
> > > buildfarm cycles and let that settle out before we start changing the
> > > answers.
> > 
> > Generally a sound plan - I started to noticeably expand it though,
> > there's some important edge cases it didn't cover.
> 
> Attached is a noticeably expanded set of tests, with fixes for the stuff
> you'd found.  I plan to push that soon-ish.  Independent of the approach
> we'll be choosing, increased coverage seems quite useful.

And for real.

  108. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-13T00:15:46Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 16:56:32 -0700, Andres Freund wrote:
>> Attached is a noticeably expanded set of tests, with fixes for the stuff
>> you'd found.  I plan to push that soon-ish.  Independent of the approach
>> we'll be choosing, increased coverage seems quite useful.

> And for real.

Looks good to me, please push.

			regards, tom lane

  109. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-13T00:32:09Z

    On 2016-09-12 20:15:46 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 16:56:32 -0700, Andres Freund wrote:
> >> Attached is a noticeably expanded set of tests, with fixes for the stuff
> >> you'd found.  I plan to push that soon-ish.  Independent of the approach
> >> we'll be choosing, increased coverage seems quite useful.
> 
> > And for real.
> 
> Looks good to me, please push.

Done.

  110. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-13T16:07:35Z

    Andres Freund <andres@anarazel.de> writes:
> Attached is a significantly updated patch series (see the mail one up
> for details about what this is, I don't want to quote it in its
> entirety).

I've reviewed the portions of 0005 that have to do with making the parser
mark queries with hasTargetSRF.  The code as you had it was wrong because
it would set the flag as a consequence of SRFs in function RTEs, which
we don't want.  It seemed to me that the best way to fix that was to rely
on the parser's p_expr_kind mechanism to tell which part of the query
we're in, whereupon we might as well make the parser act more like it does
for aggregates and window functions and give a suitable error at parse
time for misplaced SRFs.  The attached isn't perfect, in that it doesn't
know about nesting restrictions (ie that SRFs must be at top level of a
function RTE), but we could improve that later if we wanted, and anyway
it's definitely a good bit nicer than before.

This also incorporates the part of 0002 that I agree with, namely
disallowing SRFs in UPDATE, since check_srf_call_placement() naturally
would do that.

I also renamed the flag to hasTargetSRFs, which is more parallel to
hasAggs and hasWindowFuncs, and made some effort to use it in place
of expression_returns_set() searches.

I'd like to go ahead and push this, since it's a necessary prerequisite
for either approach we might adopt for the rest of the patch series,
and the improved error reporting and avoidance of expensive
expression_returns_set searches make it a win IMO even if we were not
planning to do anything more with SRFs.

			regards, tom lane

  111. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-13T16:25:05Z

    
On September 13, 2016 9:07:35 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Andres Freund <andres@anarazel.de> writes:
>> Attached is a significantly updated patch series (see the mail one up
>> for details about what this is, I don't want to quote it in its
>> entirety).
>
>I've reviewed the portions of 0005 that have to do with making the
>parser
>mark queries with hasTargetSRF.  The code as you had it was wrong
>because
>it would set the flag as a consequence of SRFs in function RTEs, which
>we don't want.

I'd taken it more as the possibility that there's an srf, than guarantee so far. There might be cases where the planner removes the srf during folding or such.  Makes sense to make it more accurate.


>  It seemed to me that the best way to fix that was to
>rely
>on the parser's p_expr_kind mechanism to tell which part of the query
>we're in, whereupon we might as well make the parser act more like it
>does
>for aggregates and window functions and give a suitable error at parse
>time for misplaced SRFs.

That's a nice improvement. The execution time errors are ugly.

>I also renamed the flag to hasTargetSRFs, which is more parallel to
>hasAggs and hasWindowFuncs, and made some effort to use it in place
>of expression_returns_set() searches.
>
>I'd like to go ahead and push this, since it's a necessary prerequisite
>for either approach we might adopt for the rest of the patch series,
>and the improved error reporting and avoidance of expensive
>expression_returns_set searches make it a win IMO even if we were not
>planning to do anything more with SRFs.

Can't look are the code just now, on my way to the airport for pgopen, but the idea sounds good to me.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  112. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-13T17:55:52Z

    Andres Freund <andres@anarazel.de> writes:
> On September 13, 2016 9:07:35 AM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'd like to go ahead and push this, since it's a necessary prerequisite
>> for either approach we might adopt for the rest of the patch series,
>> and the improved error reporting and avoidance of expensive
>> expression_returns_set searches make it a win IMO even if we were not
>> planning to do anything more with SRFs.

> Can't look are the code just now, on my way to the airport for pgopen, but the idea sounds good to me.

OK, I went ahead and pushed it.  We can tweak later if needed.

			regards, tom lane

  113. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-13T18:03:32Z

    On 2016-09-13 12:07:35 -0400, Tom Lane wrote:
> diff --git a/src/backend/optimizer/plan/analindex e28a8dc..74e4245 100644
> --- a/src/backend/optimizer/plan/analyzejoins.c
> +++ b/src/backend/optimizer/plan/analyzejoins.c
> @@ -650,6 +650,11 @@ rel_is_distinct_for(PlannerInfo *root, R
>  bool
>  query_supports_distinctness(Query *query)
>  {
> +	/* we don't cope with SRFs, see comment below */
> +	if (query->hasTargetSRFs)
> +		return false;
> +
> +	/* check for features we can prove distinctness with */
>  	if (query->distinctClause != NIL ||
>  		query->groupClause != NIL ||
>  		query->groupingSets != NIL ||
> @@ -695,7 +700,7 @@ query_is_distinct_for(Query *query, List
>  	 * specified columns, since those must be evaluated before de-duplication;
>  	 * but it doesn't presently seem worth the complication to check that.)
>  	 */
> -	if (expression_returns_set((Node *) query->targetList))
> +	if (query->hasTargetSRFs)
>  		return false;

Maybe make this hasTargetSRFs && expression_returns_set()? The SRF could
have been optimized away. (Oh, I see you recheck below. Forget that then).

> @@ -1419,8 +1419,8 @@ is_simple_subquery(Query *subquery, Rang
>  		return false;
>  
>  	/*
> -	 * Can't pull up a subquery involving grouping, aggregation, sorting,
> -	 * limiting, or WITH.  (XXX WITH could possibly be allowed later)
> +	 * Can't pull up a subquery involving grouping, aggregation, SRFs,
> +	 * sorting, limiting, or WITH.  (XXX WITH could possibly be allowed later)
>  	 *
>  	 * We also don't pull up a subquery that has explicit FOR UPDATE/SHARE
>  	 * clauses, because pullup would cause the locking to occur semantically
> @@ -1430,6 +1430,7 @@ is_simple_subquery(Query *subquery, Rang
>  	 */
>  	if (subquery->hasAggs ||
>  		subquery->hasWindowFuncs ||
> +		subquery->hasTargetSRFs ||
>  		subquery->groupClause ||
>  		subquery->groupingSets ||
>  		subquery->havingQual ||
> @@ -1543,15 +1544,6 @@ is_simple_subquery(Query *subquery, Rang
>  	}
>  
>  	/*
> -	 * Don't pull up a subquery that has any set-returning functions in its
> -	 * targetlist.  Otherwise we might well wind up inserting set-returning
> -	 * functions into places where they mustn't go, such as quals of higher
> -	 * queries.  This also ensures deletion of an empty jointree is valid.
> -	 */
> -	if (expression_returns_set((Node *) subquery->targetList))
> -		return false;

I don't quite understand parts of the comment you removed here. What
does "This also ensures deletion of an empty jointree is valid." mean?


Looks good, except that you didn't adopt the hunk adjusting
src/backend/executor/README, which still seems to read:
> We disallow set-returning functions in the targetlist of SELECT FOR UPDATE,
> so as to ensure that at most one tuple can be returned for any particular
> set of scan tuples.  Otherwise we'd get duplicates due to the original
> query returning the same set of scan tuples multiple times.  (Note: there
> is no explicit prohibition on SRFs in UPDATE, but the net effect will be
> that only the first result row of an SRF counts, because all subsequent
> rows will result in attempts to re-update an already updated target row.
> This is historical behavior and seems not worth changing.)

Regards,

Andres

  114. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-13T18:17:19Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-13 12:07:35 -0400, Tom Lane wrote:
>> /*
>> -	 * Don't pull up a subquery that has any set-returning functions in its
>> -	 * targetlist.  Otherwise we might well wind up inserting set-returning
>> -	 * functions into places where they mustn't go, such as quals of higher
>> -	 * queries.  This also ensures deletion of an empty jointree is valid.
>> -	 */
>> -	if (expression_returns_set((Node *) subquery->targetList))
>> -		return false;

> I don't quite understand parts of the comment you removed here. What
> does "This also ensures deletion of an empty jointree is valid." mean?

TBH, I don't remember what that was about anymore.  Whatever it was might
not apply now, anyway.  If there was something to it, maybe we'll
rediscover it while we're fooling with tSRFs, and then we can insert a
less cryptic comment.

> Looks good, except that you didn't adopt the hunk adjusting
> src/backend/executor/README, which still seems to read:

Ah, I missed that there was anything to change docs-wise.  Will fix.

Thanks for looking it over!

			regards, tom lane

  115. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-14T23:28:25Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-12 19:35:22 -0400, Tom Lane wrote:
>> Anyway I'll draft a prototype and then we can compare.

> Ok, cool.

Here's a draft patch that is just meant to investigate what the planner
changes might look like if we do it in the pipelined-result way.
Accordingly, I didn't touch the executor, but just had it emit regular
Result nodes for SRF-execution steps.  However, the SRFs are all
guaranteed to appear at top level of their respective tlists, so that
those Results could be replaced with something that works like
nodeFunctionscan.

A difficulty with this restriction is that if you have a query like
"select f1, generate_series(1,2) / 10 from tab" then you end up with both
a SRF-executing Result and a separate scalar-projection Result above it,
because the division-by-ten has to happen in a separate plan level.
The planner's notions about the cost of Result make it think that this is
quite expensive --- mainly because the upper Result will be iterated once
per SRF output row, so that you get hit with cpu_tuple_cost per output row.
And that in turn leads it to change plans in one or two cases in the
regression tests.  Maybe that's fine.  I'm worried though that it's right
that this will be unduly expensive.  So I'm kind of tempted to define the
SRF-executing node as acting more like, say, Agg or WindowFunc, in that
it has a list of SRFs to execute and then it has the ability to project a
scalar tlist on top of those results.  That would likely save some cycles
at execution, and it would also eliminate a few of the planner warts seen
below, like the rule about not pushing a new scalar tlist down onto a
SRF-executing Result.  I'd have to rewrite split_pathtarget_at_srfs(),
because it'd be implementing quite different rules about how to refactor
targetlists, but that's not a big problem.

On the whole I'm pretty pleased with this approach, at least from the
point of view of the planner.  The net addition of planner code is
smaller than what you had, and though I'm no doubt biased, I think this
version is much cleaner.  Also, though this patch doesn't address exactly
how we might do it, it's fairly clear that it'd be possible to allow
FDWs and CustomScans to implement SRF execution, eg pushing a SRF down to
a foreign server, in a reasonably straightforward extension of the
existing upper-pathification hooks.  If we go with the lateral function
RTE approach, that's going to be somewhere between hard and impossible.

So I think we should continue investigating this way of doing things.
I'll try to take a look at the executor end of it tomorrow.  However
I'm leaving Friday for a week's vacation, and may not have anything to
show before that.

			regards, tom lane

  116. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-15T19:23:58Z

    Andres Freund <andres@anarazel.de> writes:
> 0003-Avoid-materializing-SRFs-in-the-FROM-list.patch
>   To avoid performance regressions from moving SRFM_ValuePerCall SRFs to
>   ROWS FROM, nodeFunctionscan.c needs to support not materializing
>   output.

I looked over this patch a bit.

>   In my present patch I've *ripped out* the support for materialization
>   in nodeFunctionscan.c entirely. That means that rescans referencing
>   volatile functions can change their behaviour (if a function is
>   rescanned, without having it's parameters changed), and that native
>   backward scan support is gone.  I don't think that's actually an issue.

I think you are wrong on this not being an issue: it is critical that
rescan deliver the same results as before, else for example having a
function RTE on the inside of a nestloop will give nonsensical/broken
results.  I think what we'll have to do is allow the optimization of
skipping the tuplestore only when the function is declared nonvolatile.
(If it is, and it nonetheless gives different results on rescan, it's not
our fault if joins give haywire answers.)  I'm okay with not supporting
backward scan, but wrong answers during rescan is a different animal
entirely.

Moreover, I think we'd all agreed that this effort needs to avoid any
not-absolutely-necessary semantics changes.  This one is not only not
necessary, but it would result in subtle hard-to-detect breakage.

It's conceivable that we could allow the executor to be broken this way
and have the planner fix it by inserting a Material node when joining.
But I think it would be messy and would probably not perform as well as
an internal tuplestore --- for one thing, because the planner can't know
whether the function would return a tuplestore, making the external
materialization redundant.

Another idea is that we could extend the set of ExecInitNode flags
(EXEC_FLAG_REWIND etc) to tell child nodes whether they need to implement
rescan correctly in this sense; if they are not RHS children of nestloops,
and maybe one or two other cases, they don't.  That would give another
route by which nodeFunctionscan could decide that it can skip
materialization in common cases.

			regards, tom lane

  117. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-15T19:34:16Z

    On 2016-09-15 15:23:58 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> >   In my present patch I've *ripped out* the support for materialization
> >   in nodeFunctionscan.c entirely. That means that rescans referencing
> >   volatile functions can change their behaviour (if a function is
> >   rescanned, without having it's parameters changed), and that native
> >   backward scan support is gone.  I don't think that's actually an issue.
> 
> I think you are wrong on this not being an issue: it is critical that
> rescan deliver the same results as before, else for example having a
> function RTE on the inside of a nestloop will give nonsensical/broken
> results.

I find that quite unconvincing. We quite freely re-evaluate functions in
the targetlist again, even if they're volatile and/or SRFs.

If we implement tSRFs as pipeline nodes, we can "simply" default to the
never materializing behaviour there I guess.


> Moreover, I think we'd all agreed that this effort needs to avoid any
> not-absolutely-necessary semantics changes.

I don't agree with that. Adding pointless complications for a niche
edge cases of niche features isn't worth it.


> Another idea is that we could extend the set of ExecInitNode flags
> (EXEC_FLAG_REWIND etc) to tell child nodes whether they need to implement
> rescan correctly in this sense; if they are not RHS children of nestloops,
> and maybe one or two other cases, they don't.  That would give another
> route by which nodeFunctionscan could decide that it can skip
> materialization in common cases.

That's something I've wondered about too. Materializing if rescans are
required is quite acceptable, and probably rather rare in
practice. Seems not unlikely that that information would be valuable for
other node types too.


Regards,

Andres

  118. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-15T20:32:22Z

    Hi,

On 2016-09-14 19:28:25 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 19:35:22 -0400, Tom Lane wrote:
> >> Anyway I'll draft a prototype and then we can compare.
> 
> > Ok, cool.
> 
> Here's a draft patch that is just meant to investigate what the planner
> changes might look like if we do it in the pipelined-result way.

Nice.


> A difficulty with this restriction is that if you have a query like
> "select f1, generate_series(1,2) / 10 from tab" then you end up with both
> a SRF-executing Result and a separate scalar-projection Result above it,
> because the division-by-ten has to happen in a separate plan level.

Makes sense.  I guess we could teach the SRF pipeline node to execute a
series of such steps. Hm. That makes me think of something:

Hm. One thing I wonder about with this approach, is how we're going to
handle something absurd like:
SELECT generate_series(1, generate_series(1, 2)), generate_series(1, generate_series(2,4));

I guess we have to do here is
Step: generate_series(1,2), 1, 2, 4
Step: generate_series(1, Var(generate_series(1,2))), 1, 2, 4
Step: Var(generate_series(1, Var(generate_series(1,2)))), 1, generate_series(2, 4)
Step: Var(generate_series(1, Var(generate_series(1,2)))), generate_series(1, Var(generate_series(2, 4)))

But that'd still not have the same lockstepping behaviour, right?  I'm
at a conference, and half-ill, so I might just standing on my own brain
here.


> The planner's notions about the cost of Result make it think that this is
> quite expensive --- mainly because the upper Result will be iterated once
> per SRF output row, so that you get hit with cpu_tuple_cost per output row.
> And that in turn leads it to change plans in one or two cases in the
> regression tests.  Maybe that's fine.  I'm worried though that it's right
> that this will be unduly expensive.  So I'm kind of tempted to define the
> SRF-executing node as acting more like, say, Agg or WindowFunc, in that
> it has a list of SRFs to execute and then it has the ability to project a
> scalar tlist on top of those results.

Hah, was thinking the same above ;)


> On the whole I'm pretty pleased with this approach, at least from the
> point of view of the planner.  The net addition of planner code is
> smaller than what you had,

Not by much. But I do agree that there's some advantages here.


> and though I'm no doubt biased, I think this
> version is much cleaner.

Certainly seems a bit easier to extend and adjust behaviour. Not having
to deal with enforcing join order, and having less issues with
determining what to push where is certainly advantageous.  After all,
that was why I initially was thinking of tis approach.

> Also, though this patch doesn't address exactly
> how we might do it, it's fairly clear that it'd be possible to allow
> FDWs and CustomScans to implement SRF execution, eg pushing a SRF down to
> a foreign server, in a reasonably straightforward extension of the
> existing upper-pathification hooks.  If we go with the lateral function
> RTE approach, that's going to be somewhere between hard and impossible.

Hm. Not sure if there's that much point in doing that, but I agree that
the LATERAL approach adds more restrictions.


> So I think we should continue investigating this way of doing things.
> I'll try to take a look at the executor end of it tomorrow.  However
> I'm leaving Friday for a week's vacation, and may not have anything to
> show before that.

If you have something that's halfway recognizable, could you perhaps
post it?

Regards,

Andres

  119. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-15T20:48:59Z

    Andres Freund <andres@anarazel.de> writes:
> Hm. One thing I wonder about with this approach, is how we're going to
> handle something absurd like:
> SELECT generate_series(1, generate_series(1, 2)), generate_series(1, generate_series(2,4));

The patch that I posted would run both the generate_series(1, 2) and
generate_series(2,4) calls in the same SRF node, forcing them to run in
lockstep, after which their results would be fed to the SRF node doing
the top-level SRFs.  We could probably change it to run them in separate
nodes, but I don't see any principled way to decide which one goes first
(and in some variants of this example, it would matter).  I think the
LATERAL approach would face exactly the same issues: how many LATERAL
nodes do you use, and what's their join order?

I think we could get away with defining it like this (ie, SRFs at the same
SRF nesting level run in lockstep) as long as it's documented.  Whatever
the current behavior is for such cases would be pretty bizarre too.

			regards, tom lane

  120. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-09-15T21:08:43Z

    On 2016-09-15 16:48:59 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Hm. One thing I wonder about with this approach, is how we're going to
> > handle something absurd like:
> > SELECT generate_series(1, generate_series(1, 2)), generate_series(1, generate_series(2,4));
> 
> The patch that I posted would run both the generate_series(1, 2) and
> generate_series(2,4) calls in the same SRF node, forcing them to run in
> lockstep, after which their results would be fed to the SRF node doing
> the top-level SRFs.  We could probably change it to run them in separate
> nodes, but I don't see any principled way to decide which one goes first
> (and in some variants of this example, it would matter).

I think that's fine. I personally still think we're *much* better off
getting rid of the non-lockstep variants. You're still on the fence
about retaining the LCM behaviour (for the same nesting level at least)?


> I think the LATERAL approach would face exactly the same issues: how
> many LATERAL nodes do you use, and what's their join order?

I think this specific issue could be handled in a bit easier to grasp
variant. My PoC basically generated one RTE for each "query
level". There'd have been one RTE for generate_series(1,2), one for
gs(2,4) and one for gs(1, var(gs(1,2))), gs(1, var(gs(2,4))). Lateral
machiner would force the join order to have the argument srfs first, and
then the twoi combined srf with lateral arguments after that.

> I think we could get away with defining it like this (ie, SRFs at the same
> SRF nesting level run in lockstep) as long as it's documented.  Whatever
> the current behavior is for such cases would be pretty bizarre too.

Indeed.

Greetings,

Andres Freund

  121. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-09-15T21:12:55Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-15 16:48:59 -0400, Tom Lane wrote:
>> The patch that I posted would run both the generate_series(1, 2) and
>> generate_series(2,4) calls in the same SRF node, forcing them to run in
>> lockstep, after which their results would be fed to the SRF node doing
>> the top-level SRFs.  We could probably change it to run them in separate
>> nodes, but I don't see any principled way to decide which one goes first
>> (and in some variants of this example, it would matter).

> I think that's fine. I personally still think we're *much* better off
> getting rid of the non-lockstep variants. You're still on the fence
> about retaining the LCM behaviour (for the same nesting level at least)?

I'm happy to get rid of the LCM behavior, I just want to have some wiggle
room to be able to get it back if somebody really needs it.

			regards, tom lane

  122. Re: Changed SRF in targetlist handling

    Michael Paquier <michael.paquier@gmail.com> — 2016-10-03T03:53:26Z

    On Wed, Aug 24, 2016 at 3:55 AM, Andres Freund <andres@anarazel.de> wrote:
> Comments?

This thread has no activity for some time now and it is linked to this CF entry:
https://commitfest.postgresql.org/10/759/
I am marking it as returned with feedback..
-- 
Michael

  123. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Michael Paquier <michael.paquier@gmail.com> — 2016-10-03T03:55:35Z

    On Fri, Sep 16, 2016 at 6:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm happy to get rid of the LCM behavior, I just want to have some wiggle
> room to be able to get it back if somebody really needs it.

Er, actually no that's this thread for this CF entry:
https://commitfest.postgresql.org/10/759/
Still there has not been much activity.
-- 
Michael

  124. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2016-10-31T16:06:39Z

    Hi Tom,

On 2016-09-14 19:28:25 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 19:35:22 -0400, Tom Lane wrote:
> >> Anyway I'll draft a prototype and then we can compare.
> 
> > Ok, cool.
> 
> Here's a draft patch that is just meant to investigate what the planner
> changes might look like if we do it in the pipelined-result way.
> Accordingly, I didn't touch the executor, but just had it emit regular
> Result nodes for SRF-execution steps.  However, the SRFs are all
> guaranteed to appear at top level of their respective tlists, so that
> those Results could be replaced with something that works like
> nodeFunctionscan.

> So I think we should continue investigating this way of doing things.
> I'll try to take a look at the executor end of it tomorrow.  However
> I'm leaving Friday for a week's vacation, and may not have anything to
> show before that.

Are you planning to work on the execution side of things? I otherwise
can take a stab...

Greetings,

Andres Freund

  125. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-10-31T16:25:01Z

    Andres Freund <andres@anarazel.de> writes:
> On 2016-09-14 19:28:25 -0400, Tom Lane wrote:
>> So I think we should continue investigating this way of doing things.
>> I'll try to take a look at the executor end of it tomorrow.  However
>> I'm leaving Friday for a week's vacation, and may not have anything to
>> show before that.

> Are you planning to work on the execution side of things? I otherwise
> can take a stab...

My plan is to start on this when I go back into commitfest mode,
but right now I'm trying to produce a draft patch for RLS changes.

			regards, tom lane

  126. Re: Changed SRF in targetlist handling

    Robert Haas <robertmhaas@gmail.com> — 2016-12-08T22:18:30Z

    On Mon, Aug 22, 2016 at 4:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2016-08-17 17:41:28 -0700, Andres Freund wrote:
>>> Tom, do you think this is roughly going in the right direction?
>
> I've not had time to look at this patch, I'm afraid.  If you still
> want me to, I can make time in a day or so.

Tom, it's been about 3.5 months since you wrote this.  I think it
would be really valuable if you could get to this RSN because the
large patch set posted on the "Changed SRF in targetlist handling"
thread is backed up behind this -- and I think that's really valuable
work which I don't want to see slip out of this release.  At the same
time, both that and this are quite invasive, and I don't want it all
to get committed the day before feature freeze, because that will mess
up the schedule.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  127. Re: Changed SRF in targetlist handling

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-12-08T22:42:09Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Tom, it's been about 3.5 months since you wrote this.  I think it
> would be really valuable if you could get to this RSN because the
> large patch set posted on the "Changed SRF in targetlist handling"
> thread is backed up behind this -- and I think that's really valuable
> work which I don't want to see slip out of this release.

Yeah, I was busy with other stuff during the recent commitfest.
I'll try to get back to this.  There's still only 24 hours in a day,
though.  (And no, [1] is not enough to help.)

			regards, tom lane

[1] https://www.theguardian.com/science/2016/dec/07/earths-day-lengthens-by-two-milliseconds-a-century-astronomers-find

  128. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-16T03:29:52Z

    On 2016-10-31 09:06:39 -0700, Andres Freund wrote:
> On 2016-09-14 19:28:25 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > On 2016-09-12 19:35:22 -0400, Tom Lane wrote:
> > Here's a draft patch that is just meant to investigate what the planner
> > changes might look like if we do it in the pipelined-result way.
> > Accordingly, I didn't touch the executor, but just had it emit regular
> > Result nodes for SRF-execution steps.  However, the SRFs are all
> > guaranteed to appear at top level of their respective tlists, so that
> > those Results could be replaced with something that works like
> > nodeFunctionscan.
> 
> > So I think we should continue investigating this way of doing things.
> > I'll try to take a look at the executor end of it tomorrow.  However
> > I'm leaving Friday for a week's vacation, and may not have anything to
> > show before that.
> 
> Are you planning to work on the execution side of things? I otherwise
> can take a stab...

Doing so now.

Andres

  129. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-16T08:33:16Z

    On 2017-01-15 19:29:52 -0800, Andres Freund wrote:
> On 2016-10-31 09:06:39 -0700, Andres Freund wrote:
> > On 2016-09-14 19:28:25 -0400, Tom Lane wrote:
> > > Andres Freund <andres@anarazel.de> writes:
> > > > On 2016-09-12 19:35:22 -0400, Tom Lane wrote:
> > > Here's a draft patch that is just meant to investigate what the planner
> > > changes might look like if we do it in the pipelined-result way.
> > > Accordingly, I didn't touch the executor, but just had it emit regular
> > > Result nodes for SRF-execution steps.  However, the SRFs are all
> > > guaranteed to appear at top level of their respective tlists, so that
> > > those Results could be replaced with something that works like
> > > nodeFunctionscan.
> >
> > > So I think we should continue investigating this way of doing things.
> > > I'll try to take a look at the executor end of it tomorrow.  However
> > > I'm leaving Friday for a week's vacation, and may not have anything to
> > > show before that.
> >
> > Are you planning to work on the execution side of things? I otherwise
> > can take a stab...
>
> Doing so now.

That worked quite well.  So we have a few questions, before I clean this
up:

- For now the node is named 'Srf' both internally and in explain - not
  sure if we want to make that something longer/easier to understand for
  others? Proposals? TargetFunctionScan? SetResult?

- We could alternatively add all this into the Result node - it's not
  actually a lot of new code, and most of that is boilerplate stuff
  about adding a new node.  I'm ok with both.

- I continued with the division of Labor that Tom had set up, so we're
  creating one Srf node for each "nested" set of SRFs.  We'd discussed
  nearby to change that for one node/path for all nested SRFs, partially
  because of costing.  But I don't like the idea that much anymore. The
  implementation seems cleaner (and probably faster) this way, and I
  don't think nested targetlist SRFs are something worth optimizing
  for.  Anybody wants to argue differently?

- I chose to error out if a retset function appears in ExecEvalFunc/Oper
  and make both unconditionally set evalfunc to
  ExecMakeFunctionResultNoSets.  ExecMakeFunctionResult() is now
  externally visible.  That seems like the least noisy way to change
  things over, but I'm open for different proposals.

Comments?

Regards,

Andres

  130. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-01-16T15:17:46Z

    Andres Freund wrote:

> That worked quite well.  So we have a few questions, before I clean this
> up:
> 
> - For now the node is named 'Srf' both internally and in explain - not
>   sure if we want to make that something longer/easier to understand for
>   others? Proposals? TargetFunctionScan? SetResult?
> 
> - We could alternatively add all this into the Result node - it's not
>   actually a lot of new code, and most of that is boilerplate stuff
>   about adding a new node.  I'm ok with both.

Hmm.  I wonder if your stuff could be used as support code for
XMLTABLE[1].  Currently it has a bit of additional code of its own,
though admittedly it's very little code executor-side.  Would you mind
sharing a patch, or more details on how it works?

[1] https://www.postgresql.org/message-id/CAFj8pRA_KEukOBXvS4V-imoEEsXu0pD0AsHV0-MwRFDRWte8Lg@mail.gmail.com

> - I continued with the division of Labor that Tom had set up, so we're
>   creating one Srf node for each "nested" set of SRFs.  We'd discussed
>   nearby to change that for one node/path for all nested SRFs, partially
>   because of costing.  But I don't like the idea that much anymore. The
>   implementation seems cleaner (and probably faster) this way, and I
>   don't think nested targetlist SRFs are something worth optimizing
>   for.  Anybody wants to argue differently?

Nested targetlist SRFs make my head spin.  I suppose they may have some
use, but where would you really want this:

alvherre=# select generate_series(1, generate_series(2, 4));
 generate_series 
─────────────────
               1
               2
               1
               2
               3
               1
               2
               3
               4
(9 filas)

instead of the much more sensible

alvherre=# select i, j from generate_series(2, 4) i, generate_series(1, i) j;
 i │ j 
───┼───
 2 │ 1
 2 │ 2
 3 │ 1
 3 │ 2
 3 │ 3
 4 │ 1
 4 │ 2
 4 │ 3
 4 │ 4
(9 filas)

?  If supporting the former makes it harder to support/optimize more
reasonable cases, it seems fair game to leave them behind.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  131. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-16T18:44:17Z

    On 2017-01-16 12:17:46 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> 
> > That worked quite well.  So we have a few questions, before I clean this
> > up:
> > 
> > - For now the node is named 'Srf' both internally and in explain - not
> >   sure if we want to make that something longer/easier to understand for
> >   others? Proposals? TargetFunctionScan? SetResult?
> > 
> > - We could alternatively add all this into the Result node - it's not
> >   actually a lot of new code, and most of that is boilerplate stuff
> >   about adding a new node.  I'm ok with both.
> 
> Hmm.  I wonder if your stuff could be used as support code for
> XMLTABLE[1].

I don't immediately see what functionality overlaps, could you expand on
that?


> Currently it has a bit of additional code of its own,
> though admittedly it's very little code executor-side.  Would you mind
> sharing a patch, or more details on how it works?

Can do both; cleaning up the patch now. What we're talking about here is
a way to implement targetlist SRF that is based on:

1) a patch by Tom that creates additional Result (or now Srf) executor
   nodes containing SRF evaluation. This guarantees that only Result/Srf
   nodes have to deal with targetlist SRF evaluation.

2) new code to evaluate SRFs in the new Result/Srf node, that doesn't
   rely on ExecEvalExpr et al. to have a IsDone argument. Instead
   there's special code to handle that in the new node. That's possible
   because it's now guaranted that all SRFs are "toplevel" in the
   relevant targetlist(s).

3) Removal of all nearly tSRF related code execQual.c and other
   executor/ files, including the node->ps.ps_TupFromTlist checks
   everywhere.

makes sense?


> > - I continued with the division of Labor that Tom had set up, so we're
> >   creating one Srf node for each "nested" set of SRFs.  We'd discussed
> >   nearby to change that for one node/path for all nested SRFs, partially
> >   because of costing.  But I don't like the idea that much anymore. The
> >   implementation seems cleaner (and probably faster) this way, and I
> >   don't think nested targetlist SRFs are something worth optimizing
> >   for.  Anybody wants to argue differently?
> 
> Nested targetlist SRFs make my head spin.  I suppose they may have some
> use, but where would you really want this:

I think there's some cases where it can be useful. Targetlist SRFs as a
whole really are much more about backward compatibility than anything :)


> ?  If supporting the former makes it harder to support/optimize more
> reasonable cases, it seems fair game to leave them behind.

I don't want to desupport them, just don't want to restructure (one node
doing several levels of SRFs, instead of one per level) just to make it
easier to give good estimates.

Greetings,

Andres Freund

  132. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-01-16T19:04:34Z

    Andres Freund wrote:
> On 2017-01-16 12:17:46 -0300, Alvaro Herrera wrote:
> > Andres Freund wrote:
> > 
> > > That worked quite well.  So we have a few questions, before I clean this
> > > up:
> > > 
> > > - For now the node is named 'Srf' both internally and in explain - not
> > >   sure if we want to make that something longer/easier to understand for
> > >   others? Proposals? TargetFunctionScan? SetResult?
> > > 
> > > - We could alternatively add all this into the Result node - it's not
> > >   actually a lot of new code, and most of that is boilerplate stuff
> > >   about adding a new node.  I'm ok with both.
> > 
> > Hmm.  I wonder if your stuff could be used as support code for
> > XMLTABLE[1].
> 
> I don't immediately see what functionality overlaps, could you expand on
> that?

Well, I haven't read any previous patches in this area, but the xmltable
patch adds a new way of handling set-returning expressions, so it
appears vaguely related.  These aren't properly functions in the current
sense of the word, though.  There is some parallel to what
ExecMakeFunctionResult does, which I suppose is related.

> > Currently it has a bit of additional code of its own,
> > though admittedly it's very little code executor-side.  Would you mind
> > sharing a patch, or more details on how it works?
> 
> Can do both; cleaning up the patch now. What we're talking about here is
> a way to implement targetlist SRF that is based on:
> 
> 1) a patch by Tom that creates additional Result (or now Srf) executor
>    nodes containing SRF evaluation. This guarantees that only Result/Srf
>    nodes have to deal with targetlist SRF evaluation.
> 
> 2) new code to evaluate SRFs in the new Result/Srf node, that doesn't
>    rely on ExecEvalExpr et al. to have a IsDone argument. Instead
>    there's special code to handle that in the new node. That's possible
>    because it's now guaranted that all SRFs are "toplevel" in the
>    relevant targetlist(s).
> 
> 3) Removal of all nearly tSRF related code execQual.c and other
>    executor/ files, including the node->ps.ps_TupFromTlist checks
>    everywhere.
> 
> makes sense?

Hmm, okay.  (The ps_TupFromTlist thing has long seemed an ugly
construction.)  I think the current term for this kind of thing is
TableFunction -- are you really naming this "Srf" literally?  It seems
strange, but maybe it's just me.

> > Nested targetlist SRFs make my head spin.  I suppose they may have some
> > use, but where would you really want this:
> 
> I think there's some cases where it can be useful. Targetlist SRFs as a
> whole really are much more about backward compatibility than anything :)

Sure.

> > ?  If supporting the former makes it harder to support/optimize more
> > reasonable cases, it seems fair game to leave them behind.
> 
> I don't want to desupport them, just don't want to restructure (one node
> doing several levels of SRFs, instead of one per level) just to make it
> easier to give good estimates.

No objections.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  133. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-16T19:13:18Z

    Andres Freund <andres@anarazel.de> writes:
> That worked quite well.  So we have a few questions, before I clean this
> up:

> - For now the node is named 'Srf' both internally and in explain - not
>   sure if we want to make that something longer/easier to understand for
>   others? Proposals? TargetFunctionScan? SetResult?

"Srf" is ugly as can be, and unintelligible.  SetResult might be OK.

> - I continued with the division of Labor that Tom had set up, so we're
>   creating one Srf node for each "nested" set of SRFs.  We'd discussed
>   nearby to change that for one node/path for all nested SRFs, partially
>   because of costing.  But I don't like the idea that much anymore. The
>   implementation seems cleaner (and probably faster) this way, and I
>   don't think nested targetlist SRFs are something worth optimizing
>   for.  Anybody wants to argue differently?

Not me.

> Comments?

Hard to comment on your other points without a patch to look at.

			regards, tom lane

  134. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-16T20:52:14Z

    Hi,

On 2017-01-16 14:13:18 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > That worked quite well.  So we have a few questions, before I clean this
> > up:
>
> > - For now the node is named 'Srf' both internally and in explain - not
> >   sure if we want to make that something longer/easier to understand for
> >   others? Proposals? TargetFunctionScan? SetResult?
>
> "Srf" is ugly as can be, and unintelligible.  SetResult might be OK.

Named it SetResult - imo looks ok.  I think I do prefer the separate
node type over re-using Result.  The planner integration looks cleaner
to me due to not needing the srfpp special cases and such.


> > Comments?
>
> Hard to comment on your other points without a patch to look at.

Attached the current version. There's a *lot* of pending cleanup needed
(especially in execQual.c) removing outdated code/comments etc, but this
seems good enough for a first review.  I'd want that cleanup done in a
separate patch anyway.


Attached are two patches. The first is just a rebased version (just some
hunk offset changed) of your planner patch, on top of that is my
executor patch.  My patch moves some minor detail in yours around, and I
do think they should eventually be merged; but leaving it split for a
round displays the changes more cleanly.

Additional questions:
- do we care about SRFs that don't actually return a set? If so we need
  to change the error checking code in ExecEvalFunc/Oper and move it to
  the actual invocation.
- the FuncExpr/OpExpr check in ExecMakeFunctionResult is fairly ugly imo
  - but I don't quite see a much better solution.

Greetings,

Andres

  135. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-16T21:00:19Z

    On 2017-01-16 16:04:34 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2017-01-16 12:17:46 -0300, Alvaro Herrera wrote:
> > > Hmm.  I wonder if your stuff could be used as support code for
> > > XMLTABLE[1].
> > 
> > I don't immediately see what functionality overlaps, could you expand on
> > that?
> 
> Well, I haven't read any previous patches in this area, but the xmltable
> patch adds a new way of handling set-returning expressions, so it
> appears vaguely related.

Ugh. That's not good - I'm about to remove isDone. Like completely.
That's why I'm actually working on all this, because random expressions
returning more rows makes optimizing expression evaluation a lot harder.

> These aren't properly functions in the current sense of the word,
> though.

Why aren't they? Looks like it'd be doable to do so, at least below the
parser level?


Regards,

Andres

  136. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-01-16T21:33:30Z

    Andres Freund wrote:
> On 2017-01-16 16:04:34 -0300, Alvaro Herrera wrote:
> > Andres Freund wrote:
> > > On 2017-01-16 12:17:46 -0300, Alvaro Herrera wrote:
> > > > Hmm.  I wonder if your stuff could be used as support code for
> > > > XMLTABLE[1].
> > > 
> > > I don't immediately see what functionality overlaps, could you expand on
> > > that?
> > 
> > Well, I haven't read any previous patches in this area, but the xmltable
> > patch adds a new way of handling set-returning expressions, so it
> > appears vaguely related.
> 
> Ugh. That's not good - I'm about to remove isDone. Like completely.
> That's why I'm actually working on all this, because random expressions
> returning more rows makes optimizing expression evaluation a lot harder.

Argh.

> > These aren't properly functions in the current sense of the word,
> > though.
> 
> Why aren't they? Looks like it'd be doable to do so, at least below the
> parser level?

Hmm ...

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  137. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-17T01:00:39Z

    On 2017-01-16 12:52:14 -0800, Andres Freund wrote:
> > > Comments?
> >
> > Hard to comment on your other points without a patch to look at.
> 
> Attached the current version. There's a *lot* of pending cleanup needed
> (especially in execQual.c) removing outdated code/comments etc, but this
> seems good enough for a first review.  I'd want that cleanup done in a
> separate patch anyway.

Here's a version with a lot of that pending cleanup added (and other
light updates).  Most notably all SRF related code is gone from
executor/ excepting ExecMakeFunctionResultSet and nodeSetResult.  I'm
sure there's minor remaining references somewhere, but that's the
majority.

I think when committing this the first two patches should be combined,
but the later cleanup patch one not.  It hides too many of the actually
relevant changes.

Greetings,

Andres Freund

  138. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Robert Haas <robertmhaas@gmail.com> — 2017-01-17T17:05:04Z

    On Mon, Jan 16, 2017 at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> That worked quite well.  So we have a few questions, before I clean this
>> up:
>
>> - For now the node is named 'Srf' both internally and in explain - not
>>   sure if we want to make that something longer/easier to understand for
>>   others? Proposals? TargetFunctionScan? SetResult?
>
> "Srf" is ugly as can be, and unintelligible.  SetResult might be OK.

The operation we're performing here, IIUC, is projection.  SetResult
lacks a verb, although Set could be confused with one; someone might
think this is the node that sets a result, whatever that means.
Anyway, I suggest working Project in there somehow.  If Project by
itself seems like it's too generic, perhaps ProjectSet or
ProjectSetResult would be suitable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  139. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-17T17:52:20Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jan 16, 2017 at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> "Srf" is ugly as can be, and unintelligible.  SetResult might be OK.

> The operation we're performing here, IIUC, is projection.  SetResult
> lacks a verb, although Set could be confused with one; someone might
> think this is the node that sets a result, whatever that means.
> Anyway, I suggest working Project in there somehow.  If Project by
> itself seems like it's too generic, perhaps ProjectSet or
> ProjectSetResult would be suitable.

Andres' patch is already using "SetProjectionPath" for the path struct
type.  Maybe make that "ProjectSetPath", giving rise to a "ProjectSet"
plan node?

I'm happy to do a global-search-and-replace while I'm reviewing the
patch, but let's decide on names PDQ.

			regards, tom lane

  140. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Robert Haas <robertmhaas@gmail.com> — 2017-01-17T17:58:38Z

    On Tue, Jan 17, 2017 at 12:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Jan 16, 2017 at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> "Srf" is ugly as can be, and unintelligible.  SetResult might be OK.
>
>> The operation we're performing here, IIUC, is projection.  SetResult
>> lacks a verb, although Set could be confused with one; someone might
>> think this is the node that sets a result, whatever that means.
>> Anyway, I suggest working Project in there somehow.  If Project by
>> itself seems like it's too generic, perhaps ProjectSet or
>> ProjectSetResult would be suitable.
>
> Andres' patch is already using "SetProjectionPath" for the path struct
> type.  Maybe make that "ProjectSetPath", giving rise to a "ProjectSet"
> plan node?

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  141. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-17T18:03:24Z

    Hi,

On 2017-01-17 12:52:20 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, Jan 16, 2017 at 2:13 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> "Srf" is ugly as can be, and unintelligible.  SetResult might be OK.
>
> > The operation we're performing here, IIUC, is projection.  SetResult
> > lacks a verb, although Set could be confused with one; someone might
> > think this is the node that sets a result, whatever that means.
> > Anyway, I suggest working Project in there somehow.  If Project by
> > itself seems like it's too generic, perhaps ProjectSet or
> > ProjectSetResult would be suitable.

I'd not have gone for SetResult if we didn't already have Result.  I'm
not super happy ending up having Project in ProjectSet but not in the
Result that end up doing the majority of the projection.  But eh, we can
live with it.


> Andres' patch is already using "SetProjectionPath" for the path struct
> type.  Maybe make that "ProjectSetPath", giving rise to a "ProjectSet"
> plan node?

WFM.


> I'm happy to do a global-search-and-replace while I'm reviewing the
> patch, but let's decide on names PDQ.

Yes, let's decide soon please.


Greeting,

Andres

  142. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-17T18:18:02Z

    Andres Freund <andres@anarazel.de> writes:
> I'd not have gone for SetResult if we didn't already have Result.  I'm
> not super happy ending up having Project in ProjectSet but not in the
> Result that end up doing the majority of the projection.  But eh, we can
> live with it.

Using Result for two completely different things is a wart though.  If we
had it to do over I think we'd define Result as a scan node that produces
rows from no input, and create a separate Project node for the case of
projecting from input tuples.  People are used to seeing Result in EXPLAIN
output, so it's not worth the trouble of changing that IMO, but we don't
have to use it as a model for more node types.

			regards, tom lane

  143. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Robert Haas <robertmhaas@gmail.com> — 2017-01-17T18:20:52Z

    On Tue, Jan 17, 2017 at 1:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> I'd not have gone for SetResult if we didn't already have Result.  I'm
>> not super happy ending up having Project in ProjectSet but not in the
>> Result that end up doing the majority of the projection.  But eh, we can
>> live with it.
>
> Using Result for two completely different things is a wart though.  If we
> had it to do over I think we'd define Result as a scan node that produces
> rows from no input, and create a separate Project node for the case of
> projecting from input tuples.  People are used to seeing Result in EXPLAIN
> output, so it's not worth the trouble of changing that IMO, but we don't
> have to use it as a model for more node types.

+1, although I think changing the existing node would be fine too if
somebody wanted to do the work.  It's not worth having that wart
forever just to avoid whatever minor pain-of-adjustment might be
involved.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  144. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-17T18:43:38Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 17, 2017 at 1:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Using Result for two completely different things is a wart though.  If we
>> had it to do over I think we'd define Result as a scan node that produces
>> rows from no input, and create a separate Project node for the case of
>> projecting from input tuples.  People are used to seeing Result in EXPLAIN
>> output, so it's not worth the trouble of changing that IMO, but we don't
>> have to use it as a model for more node types.

> +1, although I think changing the existing node would be fine too if
> somebody wanted to do the work.  It's not worth having that wart
> forever just to avoid whatever minor pain-of-adjustment might be
> involved.

Although ... looking closer at Andres' patch, the new node type *is*
channeling Result, in the sense that it might or might not have any input
plan.  This probably traces to what I wrote in September:

+     * XXX Possibly-temporary hack: if the subpath is a dummy ResultPath,
+     * don't bother with it, just make a Result with no input.  This avoids an
+     * extra Result plan node when doing "SELECT srf()".  Depending on what we
+     * decide about the desired plan structure for SRF-expanding nodes, this
+     * optimization might have to go away, and in any case it'll probably look
+     * a good bit different.

I'm not convinced that that optimization is worth preserving, but if we
keep it then ProjectSet isn't le mot juste here, any more than you'd want
to rename Result to Project without changing its existing functionality.

			regards, tom lane

  145. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-17T18:53:46Z

    On 2017-01-17 13:43:38 -0500, Tom Lane wrote:
> Although ... looking closer at Andres' patch, the new node type *is*
> channeling Result, in the sense that it might or might not have any input
> plan.  This probably traces to what I wrote in September:
>
> +     * XXX Possibly-temporary hack: if the subpath is a dummy ResultPath,
> +     * don't bother with it, just make a Result with no input.  This avoids an
> +     * extra Result plan node when doing "SELECT srf()".  Depending on what we
> +     * decide about the desired plan structure for SRF-expanding nodes, this
> +     * optimization might have to go away, and in any case it'll probably look
> +     * a good bit different.
>
> I'm not convinced that that optimization is worth preserving, but if we
> keep it then ProjectSet isn't le mot juste here, any more than you'd want
> to rename Result to Project without changing its existing
> functionality.

Right. I'd removed that, and re-added it; primarily because the plans
looked more complex without it. After all, you'd thought it worth adding
that hack ;)   I'm happy with removing it again too.

Andres

  146. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-17T19:15:21Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-01-17 13:43:38 -0500, Tom Lane wrote:
>> I'm not convinced that that optimization is worth preserving, but if we
>> keep it then ProjectSet isn't le mot juste here, any more than you'd want
>> to rename Result to Project without changing its existing
>> functionality.

> Right. I'd removed that, and re-added it; primarily because the plans
> looked more complex without it. After all, you'd thought it worth adding
> that hack ;)   I'm happy with removing it again too.

Well, it seemed reasonable to do that as long as the only cost was ten or
so lines in create_projection_plan.  But if we're contorting not only the
behavior but the very name of the SRF-evaluation plan node type, that's
not negligible cost anymore.  So now I'm inclined to take it out.

			regards, tom lane

  147. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-18T13:43:24Z

    I did a review pass over 0001 and 0002.  I think the attached updated
version is committable ... except for one thing.  The more I look at it,
the more disturbed I am by the behavioral change shown in rangefuncs.out
--- that's the SRF-in-one-arm-of-CASE issue.  (The changes in tsrf.out
are fine and as per agreement.)  We touched lightly on that point far
upthread, but didn't really resolve it.  What's bothering me is that
we're changing, silently, from a reasonably-intuitive behavior to a
completely-not-intuitive one.  Since we got a bug report for the previous
less-than-intuitive behavior for such cases, it's inevitable that we'll
get bug reports for this.  I think it'd be far better to throw error for
SRF-inside-a-CASE.  If we don't, we certainly need to document this,
and I'm not very sure how to explain it clearly.

Upthread we had put COALESCE in the same bucket, but I think that's less
of a problem, because in typical usages the SRF would be in the first
argument and so users wouldn't be expecting conditional evaluation.

Anyway, I've not done anything about that in the attached.  What I did do:

* Merge 0001 and 0002.  I appreciate you having separated that for my
review, but it doesn't make any sense to commit the parts of 0001 that
you undid in 0002.

* Rename the node types as per yesterday's discussion.

* Require Project to always have an input plan node.

* Obviously, ExecMakeFunctionResultSet can be greatly simplified now
that it need not deal with hasSetArg cases.  I saw you'd left that
for later, which is mostly fine, but I did lobotomize it just enough
to throw an error if it gets a set result from an argument.  Without
that, we wouldn't really be testing that the planner splits nested
SRFs correctly.

* This bit in ExecProjectSRF was no good:

+         else if (IsA(gstate->arg, FuncExprState) &&
+                  ((FuncExpr *) gstate->arg->expr)->funcretset)

because FuncExprState is used for more node types than just FuncExpr;
in particular this would fail (except perhaps by accident) for a
set-returning OpExpr.  I chose to fix it by adding a funcReturnsSet
field to FuncExprState and insisting that ExecInitExpr fill that in
immediately, which it can do easily.

* Minor style and comment improvements; fix a couple small oversights
such as missing outfuncs.c support.

* Update the user documentation (didn't address the CASE issue, though).

			regards, tom lane

  148. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-18T19:19:24Z

    Hi,

On 2017-01-18 08:43:24 -0500, Tom Lane wrote:
> I did a review pass over 0001 and 0002.  I think the attached updated
> version is committable

Cool.

> ... except for one thing.  The more I look at it,
> the more disturbed I am by the behavioral change shown in rangefuncs.out
> --- that's the SRF-in-one-arm-of-CASE issue.  (The changes in tsrf.out
> are fine and as per agreement.)  We touched lightly on that point far
> upthread, but didn't really resolve it.  What's bothering me is that
> we're changing, silently, from a reasonably-intuitive behavior to a
> completely-not-intuitive one.  Since we got a bug report for the previous
> less-than-intuitive behavior for such cases, it's inevitable that we'll
> get bug reports for this.  I think it'd be far better to throw error for
> SRF-inside-a-CASE.  If we don't, we certainly need to document this,
> and I'm not very sure how to explain it clearly.

I'm fine with leaving it as is in the patch, but I'm also fine with
changing things to ERROR.  Personally I don't think it matters much, and
we can whack it back and forth as we want later.  Thus I'm inclined to
commit it without erroring out; since presumably we'll take some time
deciding on what exactly we want to prohibit.


> Anyway, I've not done anything about that in the attached.  What I did do:
> 
> * Merge 0001 and 0002.  I appreciate you having separated that for my
> review, but it doesn't make any sense to commit the parts of 0001 that
> you undid in 0002.

Right. I was suggesting upthread that we'd merge them before committing.


> * Obviously, ExecMakeFunctionResultSet can be greatly simplified now
> that it need not deal with hasSetArg cases.

Yea, I've cleaned it up in my 0003; where it would have started to error
out too (without an explicit check), because there's no set evaluating
function anymore besides ExecMakeFunctionResultSet.


> I saw you'd left that
> for later, which is mostly fine, but I did lobotomize it just enough
> to throw an error if it gets a set result from an argument.  Without
> that, we wouldn't really be testing that the planner splits nested
> SRFs correctly.

Ok, that makes sense.


> * This bit in ExecProjectSRF was no good:
> 
> +         else if (IsA(gstate->arg, FuncExprState) &&
> +                  ((FuncExpr *) gstate->arg->expr)->funcretset)
> 
> because FuncExprState is used for more node types than just FuncExpr;
> in particular this would fail (except perhaps by accident) for a
> set-returning OpExpr.

Argh. That should have been FunExprState->func->fn_retset.  Anyway, your
approach works, too.


> * Update the user documentation (didn't address the CASE issue, though).

Cool.


Greetings,

Andres

  149. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-18T19:24:12Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-01-18 08:43:24 -0500, Tom Lane wrote:
>> ... except for one thing.  The more I look at it,
>> the more disturbed I am by the behavioral change shown in rangefuncs.out
>> --- that's the SRF-in-one-arm-of-CASE issue.

> I'm fine with leaving it as is in the patch, but I'm also fine with
> changing things to ERROR.  Personally I don't think it matters much, and
> we can whack it back and forth as we want later.  Thus I'm inclined to
> commit it without erroring out; since presumably we'll take some time
> deciding on what exactly we want to prohibit.

I agree.  If we do decide to throw an error, it would best be done in
parse analysis, and thus would be practically independent of this patch
anyway.

>> * This bit in ExecProjectSRF was no good:
>> +         else if (IsA(gstate->arg, FuncExprState) &&
>> +                  ((FuncExpr *) gstate->arg->expr)->funcretset)

> Argh. That should have been FunExprState->func->fn_retset.

Nope; that was my first thought as well, but fn_retset isn't valid if
init_fcache hasn't been run yet, which it won't have been the first time
through.

So I think we can push this patch now and get on with the downstream
patches.  Do you want to do the honors, or shall I?

			regards, tom lane

  150. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-18T19:34:20Z

    Hi,

On 2017-01-18 14:24:12 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-01-18 08:43:24 -0500, Tom Lane wrote:
> >> ... except for one thing.  The more I look at it,
> >> the more disturbed I am by the behavioral change shown in rangefuncs.out
> >> --- that's the SRF-in-one-arm-of-CASE issue.
> 
> > I'm fine with leaving it as is in the patch, but I'm also fine with
> > changing things to ERROR.  Personally I don't think it matters much, and
> > we can whack it back and forth as we want later.  Thus I'm inclined to
> > commit it without erroring out; since presumably we'll take some time
> > deciding on what exactly we want to prohibit.
> 
> I agree.  If we do decide to throw an error, it would best be done in
> parse analysis, and thus would be practically independent of this patch
> anyway.

Cool, agreed then.


> >> * This bit in ExecProjectSRF was no good:
> >> +         else if (IsA(gstate->arg, FuncExprState) &&
> >> +                  ((FuncExpr *) gstate->arg->expr)->funcretset)
> 
> > Argh. That should have been FunExprState->func->fn_retset.
> 
> Nope; that was my first thought as well, but fn_retset isn't valid if
> init_fcache hasn't been run yet, which it won't have been the first time
> through.

Righty-O :(


> So I think we can push this patch now and get on with the downstream
> patches.  Do you want to do the honors, or shall I?

Whatever you prefer - either way, I'll go on to rebasing the cleanup
patch afterwards (whose existance should probably be mentioned in the
commit message).


Greetings,

Andres Freund

  151. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-18T20:00:12Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-01-18 14:24:12 -0500, Tom Lane wrote:
>> So I think we can push this patch now and get on with the downstream
>> patches.  Do you want to do the honors, or shall I?

> Whatever you prefer - either way, I'll go on to rebasing the cleanup
> patch afterwards (whose existance should probably be mentioned in the
> commit message).

OK, I can do it --- I have the revised patch already queued up in git
stash, so it's easy.  Need to write a commit msg though.  Did you have
a draft for that?

			regards, tom lane

  152. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-18T20:11:47Z

    Hi,

On January 18, 2017 12:00:12 PM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>Andres Freund <andres@anarazel.de> writes:
>> On 2017-01-18 14:24:12 -0500, Tom Lane wrote:
>>> So I think we can push this patch now and get on with the downstream
>>> patches.  Do you want to do the honors, or shall I?
>
>> Whatever you prefer - either way, I'll go on to rebasing the cleanup
>> patch afterwards (whose existance should probably be mentioned in the
>> commit message).
>
>OK, I can do it --- I have the revised patch already queued up in git
>stash, so it's easy.  Need to write a commit msg though.  Did you have
>a draft for that?

Yea, have something lying around.  Let me push it then when I get back from lunch?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  153. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-18T20:24:32Z

    Andres Freund <andres@anarazel.de> writes:
> Yea, have something lying around.  Let me push it then when I get back from lunch?

Sure, no sweat.

			regards, tom lane

  154. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-18T21:47:02Z

    Hi,

On 2017-01-18 15:24:32 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Yea, have something lying around.  Let me push it then when I get back from lunch?
> 
> Sure, no sweat.

Pushed.  Yay!

There's one sgml comment you'd added:
"Furthermore, nested set-returning functions did not work at all."

I'm not quite sure what you're referring to there - it was previously
allowed to have one set argument to an SRF:

postgres[28758][1]=# SELECT generate_series(1,generate_series(1,5));
┌─────────────────┐
│ generate_series │
├─────────────────┤
│               1 │
│               1 │
│               2 │
│               1 │
│               2 │
│               3 │


Am I misunderstanding what you meant?  I left it in what I committed,
but we probably should clear up the language there.


Working on rebasing the cleanup patch now.  Interested in reviewing
that?  Otherwise I think I'll just push the rebased version of what I'd
posted before, after making another pass through it.


- Andres

  155. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-18T21:56:46Z

    Andres Freund <andres@anarazel.de> writes:
> There's one sgml comment you'd added:
> "Furthermore, nested set-returning functions did not work at all."
> I'm not quite sure what you're referring to there - it was previously
> allowed to have one set argument to an SRF:

Ooops ... that was composed too hastily, evidently.  Will fix.

I'll try to write something about the SRF-in-CASE issue too.  Seeing
whether we can document that adequately seems like an important part
of making the decision about whether we need to block it.

> Working on rebasing the cleanup patch now.  Interested in reviewing
> that?  Otherwise I think I'll just push the rebased version of what I'd
> posted before, after making another pass through it.

I have not actually looked at 0003 at all yet.  So yeah, please post
for review after you're done rebasing.

			regards, tom lane

  156. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-18T22:11:54Z

    On 2017-01-18 16:56:46 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> I have not actually looked at 0003 at all yet.  So yeah, please post
> for review after you're done rebasing.

Here's a rebased and lightly massaged version. I'm vanishing in a
meeting for a bit, thought it'd be more useful to get it now rather than
later.

(I also noticed the previous patch should have had a catversion bump :(,
will do after the meeting).

- Andres

  157. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-18T22:34:56Z

    Andres Freund <andres@anarazel.de> writes:
> (I also noticed the previous patch should have had a catversion bump :(,
> will do after the meeting).

Uh, why?  It isn't touching any on-disk data structure.

			regards, tom lane

  158. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-18T22:58:31Z

    On 2017-01-18 17:34:56 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > (I also noticed the previous patch should have had a catversion bump :(,
> > will do after the meeting).
> 
> Uh, why?  It isn't touching any on-disk data structure.

Forget what I said - I was rushing to a meeting and not thinking
entirely clearly.  Was thinking about the new node types and that we now
(de)serialize plans for parallelism - but that's guaranteed to be the
same version.

Andres

  159. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-18T23:14:26Z

    I wrote:
> I'll try to write something about the SRF-in-CASE issue too.  Seeing
> whether we can document that adequately seems like an important part
> of making the decision about whether we need to block it.

Here's what I came up with:

  This behavior also means that set-returning functions will be evaluated
  even when it might appear that they should be skipped because of a
  conditional-evaluation construct, such as CASE or COALESCE. For example,
  consider

  SELECT x, CASE WHEN x > 0 THEN generate_series(1, 5) ELSE 0 END FROM tab;

  It might seem that this should produce five repetitions of input rows
  that have x > 0, and a single repetition of those that do not; but
  actually it will produce five repetitions of every input row. This is
  because generate_series() is run first, and then the CASE expression is
  applied to its result rows. The behavior is thus comparable to

  SELECT x, CASE WHEN x > 0 THEN g ELSE 0 END
    FROM tab, LATERAL generate_series(1,5) AS g;

  It would be exactly the same, except that in this specific example, the
  planner could choose to put g on the outside of the nestloop join, since
  g has no actual lateral dependency on tab. That would result in a
  different output row order. Set-returning functions in the select list
  are always evaluated as though they are on the inside of a nestloop join
  with the rest of the FROM clause, so that the function(s) are run to
  completion before the next row from the FROM clause is considered.

So is this too ugly to live, or shall we put up with it?

			regards, tom lane

  160. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-18T23:19:45Z

    On 2017-01-18 18:14:26 -0500, Tom Lane wrote:
> I wrote:
> > I'll try to write something about the SRF-in-CASE issue too.  Seeing
> > whether we can document that adequately seems like an important part
> > of making the decision about whether we need to block it.
> 
> Here's what I came up with:
> 
>   This behavior also means that set-returning functions will be evaluated
>   even when it might appear that they should be skipped because of a
>   conditional-evaluation construct, such as CASE or COALESCE. For example,
>   consider
> 
>   SELECT x, CASE WHEN x > 0 THEN generate_series(1, 5) ELSE 0 END FROM tab;
> 
>   It might seem that this should produce five repetitions of input rows
>   that have x > 0, and a single repetition of those that do not; but
>   actually it will produce five repetitions of every input row. This is
>   because generate_series() is run first, and then the CASE expression is
>   applied to its result rows. The behavior is thus comparable to
> 
>   SELECT x, CASE WHEN x > 0 THEN g ELSE 0 END
>     FROM tab, LATERAL generate_series(1,5) AS g;
> 
>   It would be exactly the same, except that in this specific example, the
>   planner could choose to put g on the outside of the nestloop join, since
>   g has no actual lateral dependency on tab. That would result in a
>   different output row order. Set-returning functions in the select list
>   are always evaluated as though they are on the inside of a nestloop join
>   with the rest of the FROM clause, so that the function(s) are run to
>   completion before the next row from the FROM clause is considered.
> 
> So is this too ugly to live, or shall we put up with it?

I'm very tentatively in favor of living with it.

Greetings,

Andres Freund

  161. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    David G. Johnston <david.g.johnston@gmail.com> — 2017-01-18T23:27:53Z

    On Wed, Jan 18, 2017 at 4:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > I'll try to write something about the SRF-in-CASE issue too.  Seeing
> > whether we can document that adequately seems like an important part
> > of making the decision about whether we need to block it.
>
> Here's what I came up with:
>
>   This behavior also means that set-returning functions will be evaluated
>   even when it might appear that they should be skipped because of a
>   conditional-evaluation construct, such as CASE or COALESCE. For example,
>   consider
>
>   SELECT x, CASE WHEN x > 0 THEN generate_series(1, 5) ELSE 0 END FROM tab;
>
>   It might seem that this should produce five repetitions of input rows
>   that have x > 0, and a single repetition of those that do not; but
>   actually it will produce five repetitions of every input row.
>
> So is this too ugly to live, or shall we put up with it?
>
>
​Disallowing such an unlikely, and un-intuitive, corner-case strikes my
sensibilities.

​I'd rather fail now and allow for the possibility of future implementation
of the "it might seem that..." behavior.​

David J.

  162. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-18T23:39:40Z

    On 2017-01-18 16:27:53 -0700, David G. Johnston wrote:
> ​I'd rather fail now and allow for the possibility of future implementation
> of the "it might seem that..." behavior.​

That's very unlikely to happen.

  163. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Robert Haas <robertmhaas@gmail.com> — 2017-01-18T23:59:00Z

    On Wed, Jan 18, 2017 at 6:19 PM, Andres Freund <andres@anarazel.de> wrote:
>>   SELECT x, CASE WHEN x > 0 THEN generate_series(1, 5) ELSE 0 END FROM tab;
>>
>>   It might seem that this should produce five repetitions of input rows
>>   that have x > 0, and a single repetition of those that do not; but
>>   actually it will produce five repetitions of every input row. This is
>>   because generate_series() is run first, and then the CASE expression is
>>   applied to its result rows. The behavior is thus comparable to
>>
>>   SELECT x, CASE WHEN x > 0 THEN g ELSE 0 END
>>     FROM tab, LATERAL generate_series(1,5) AS g;
>>
>>   It would be exactly the same, except that in this specific example, the
>>   planner could choose to put g on the outside of the nestloop join, since
>>   g has no actual lateral dependency on tab. That would result in a
>>   different output row order. Set-returning functions in the select list
>>   are always evaluated as though they are on the inside of a nestloop join
>>   with the rest of the FROM clause, so that the function(s) are run to
>>   completion before the next row from the FROM clause is considered.
>>
>> So is this too ugly to live, or shall we put up with it?
>
> I'm very tentatively in favor of living with it.

So, one of the big reasons I use CASE is to avoid evaluating
expressions in cases where they might throw an ERROR.  Like, you know:

CASE WHEN d != 0 THEN n / d ELSE NULL END

I guess it's not the end of the world if that only works for
non-set-returning functions, but it's something to think about.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  164. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-19T00:00:43Z

    
On January 18, 2017 3:59:00 PM PST, Robert Haas <robertmhaas@gmail.com> wrote:
>On Wed, Jan 18, 2017 at 6:19 PM, Andres Freund <andres@anarazel.de>
>wrote:
>>>   SELECT x, CASE WHEN x > 0 THEN generate_series(1, 5) ELSE 0 END
>FROM tab;
>>>
>>>   It might seem that this should produce five repetitions of input
>rows
>>>   that have x > 0, and a single repetition of those that do not; but
>>>   actually it will produce five repetitions of every input row. This
>is
>>>   because generate_series() is run first, and then the CASE
>expression is
>>>   applied to its result rows. The behavior is thus comparable to
>>>
>>>   SELECT x, CASE WHEN x > 0 THEN g ELSE 0 END
>>>     FROM tab, LATERAL generate_series(1,5) AS g;
>>>
>>>   It would be exactly the same, except that in this specific
>example, the
>>>   planner could choose to put g on the outside of the nestloop join,
>since
>>>   g has no actual lateral dependency on tab. That would result in a
>>>   different output row order. Set-returning functions in the select
>list
>>>   are always evaluated as though they are on the inside of a
>nestloop join
>>>   with the rest of the FROM clause, so that the function(s) are run
>to
>>>   completion before the next row from the FROM clause is considered.
>>>
>>> So is this too ugly to live, or shall we put up with it?
>>
>> I'm very tentatively in favor of living with it.
>
>So, one of the big reasons I use CASE is to avoid evaluating
>expressions in cases where they might throw an ERROR.  Like, you know:
>
>CASE WHEN d != 0 THEN n / d ELSE NULL END
>
>I guess it's not the end of the world if that only works for
>non-set-returning functions, but it's something to think about.

That's already not reliable in a bunch of cases, particularly evaluation during planning...  Not saying that's good, but it is.

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  165. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Robert Haas <robertmhaas@gmail.com> — 2017-01-19T00:05:52Z

    On Wed, Jan 18, 2017 at 7:00 PM, Andres Freund <andres@anarazel.de> wrote:
>>So, one of the big reasons I use CASE is to avoid evaluating
>>expressions in cases where they might throw an ERROR.  Like, you know:
>>
>>CASE WHEN d != 0 THEN n / d ELSE NULL END
>>
>>I guess it's not the end of the world if that only works for
>>non-set-returning functions, but it's something to think about.
>
> That's already not reliable in a bunch of cases, particularly evaluation during planning...  Not saying that's good, but it is.

Whee!

:-)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  166. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-19T00:08:04Z

    Robert Haas <robertmhaas@gmail.com> writes:
> So, one of the big reasons I use CASE is to avoid evaluating
> expressions in cases where they might throw an ERROR.  Like, you know:
> CASE WHEN d != 0 THEN n / d ELSE NULL END
> I guess it's not the end of the world if that only works for
> non-set-returning functions, but it's something to think about.

Well, refusing CASE-containing-SRF at all isn't going to make your
life any better in that regard :-(

It's possibly worth noting that this is also true for aggregates and
window functions: wrapping those in a CASE doesn't stop them from being
evaluated, either.  People seem to be generally used to that, although
I think I've seen one or two complaints about it from folks who seemed
unclear on the concept of aggregates.

In the end I think this is mostly about backwards compatibility:
are we sufficiently worried about that that we'd rather throw an
error than have a silent change of behavior?  TBH I'm not sure.
We've certainly got two other silent changes of behavior in this
same patch.  The argument for treating this one differently,
I think, is that it's changing from a less surprising behavior
to a more surprising one whereas the other changes are the reverse,
or at worst neutral.

			regards, tom lane

  167. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-19T18:06:20Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-01-18 16:56:46 -0500, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>> I have not actually looked at 0003 at all yet.  So yeah, please post
>> for review after you're done rebasing.

> Here's a rebased and lightly massaged version.

I've read through this and made some minor improvements, mostly additional
comment cleanup.  One thing I wanted to ask about:

@@ -4303,7 +4303,7 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
 
     /*
      * Forget it if the function is not SQL-language or has other showstopper
-     * properties.  (The nargs check is just paranoia.)
+     * properties.  (The nargs and retset checks are just paranoia.)
      */
     if (funcform->prolang != SQLlanguageId ||
         funcform->prosecdef ||

I thought this change was simply wrong, and removed it; AFAIK it's
perfectly possible to get here for set-returning functions, since
the planner does expression simplification long before it worries
about splitting out SRFs.  Did you have a reason to think differently?

Other than that possible point, I think the attached is committable.

			regards, tom lane

  168. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Andres Freund <andres@anarazel.de> — 2017-01-19T22:06:22Z

    On 2017-01-19 13:06:20 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-01-18 16:56:46 -0500, Tom Lane wrote:
> >> Andres Freund <andres@anarazel.de> writes:
> >> I have not actually looked at 0003 at all yet.  So yeah, please post
> >> for review after you're done rebasing.
>
> > Here's a rebased and lightly massaged version.
>
> I've read through this and made some minor improvements, mostly additional
> comment cleanup.

Thanks!


> One thing I wanted to ask about:
>
> @@ -4303,7 +4303,7 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
>
>      /*
>       * Forget it if the function is not SQL-language or has other showstopper
> -     * properties.  (The nargs check is just paranoia.)
> +     * properties.  (The nargs and retset checks are just paranoia.)
>       */
>      if (funcform->prolang != SQLlanguageId ||
>          funcform->prosecdef ||
>
> I thought this change was simply wrong, and removed it;

Hm. I made that change a while ago.  It might have been a holdover from
the old approach, where it'd indeed have been impossible to see any
tSRFs here.  Or it might have been because we check
querytree->hasTargetSRFs below (which should prevent inlining a function
that actually returns multiple rows).  I agree it's better to leave the
check there. Maybe we ought to remove the paranoia bit about retset
though - it's not paranoia if it has an effect.


> Other than that possible point, I think the attached is committable.

Will do so in a bit, after a s/and retset checks are/check is/. And then
fix that big-endian ordering issue.

- Andres

  169. Re: Implement targetlist SRFs using ROWS FROM() (was Changed SRF in targetlist handling)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-19T23:22:33Z

    Andres Freund <andres@anarazel.de> writes:
> Maybe we ought to remove the paranoia bit about retset
> though - it's not paranoia if it has an effect.

Exactly, and I already did that in my version.

			regards, tom lane