Re: Changed SRF in targetlist handling

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

On 23 May 2016 at 08:53, Andres Freund <andres(at)anarazel(dot)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

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(dot)1458767514(at)sss(dot)pgh(dot)pa(dot)us#25750(dot)1458767514@sss.pgh.pa.us

On Sun, May 22, 2016 at 8:53 PM, Andres Freund <andres(at)anarazel(dot)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.

Andres Freund <andres(at)anarazel(dot)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

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

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

David Fetter <david(at)fetter(dot)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

On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote:
> David Fetter <david(at)fetter(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

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

David Fetter <david(at)fetter(dot)org> writes:
> On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote:
>> David Fetter <david(at)fetter(dot)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

On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)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

On Mon, May 23, 2016 at 1:44 PM, David Fetter <david(at)fetter(dot)org> wrote:

> On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote:
> > David Fetter <david(at)fetter(dot)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.​

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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Andres Freund <andres(at)anarazel(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

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

On Mon, May 23, 2016 at 2:13 PM, David Fetter <david(at)fetter(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> > Andres Freund <andres(at)anarazel(dot)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

On 05/23/2016 12:39 PM, Merlin Moncure wrote:
> On Mon, May 23, 2016 at 2:13 PM, David Fetter <david(at)fetter(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Andres Freund <andres(at)anarazel(dot)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

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(at)fetter(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> > Andres Freund <andres(at)anarazel(dot)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(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

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

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> On Mon, May 23, 2016 at 2:13 PM, David Fetter <david(at)fetter(dot)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

On Mon, May 23, 2016 at 4:05 PM, David Fetter <david(at)fetter(dot)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(at)fetter(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >> > Andres Freund <andres(at)anarazel(dot)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.

Joe Conway <mail(at)joeconway(dot)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

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)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

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

> Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> > On Mon, May 23, 2016 at 2:13 PM, David Fetter <david(at)fetter(dot)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.​

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

> Tom Lane wrote:
> > Joe Conway <mail(at)joeconway(dot)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.

"David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> On Mon, May 23, 2016 at 4:24 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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

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

> "David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> > On Mon, May 23, 2016 at 4:24 PM, Alvaro Herrera <
> alvherre(at)2ndquadrant(dot)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.​

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

On 2016-05-23 13:10:29 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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).

Andres Freund <andres(at)anarazel(dot)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

On 2016-05-25 15:02:23 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

On 2016-05-25 15:20:03 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

On Wed, May 25, 2016 at 3:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)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

Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> On Wed, May 25, 2016 at 3:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Andres Freund <andres(at)anarazel(dot)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

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

> Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
> > On Wed, May 25, 2016 at 3:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Andres Freund <andres(at)anarazel(dot)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.

"David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> On Friday, June 3, 2016, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us
> <javascript:_e(%7B%7D,'cvml','tgl(at)sss(dot)pgh(dot)pa(dot)us');>> wrote:
>> Merlin Moncure <mmoncure(at)gmail(dot)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

On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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

> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.

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

"David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)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

On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Robert Haas wrote:
> On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Mon, Jun 6, 2016 at 2:31 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Robert Haas wrote:
> > On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > >> On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Robert Haas wrote:
>> On Mon, Jun 6, 2016 at 11:50 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Mon, Jun 6, 2016 at 2:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

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

> Robert Haas <robertmhaas(at)gmail(dot)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.

On Mon, Jun 6, 2016 at 3:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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

On 2016-05-25 16:55:23 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

On 2016-08-02 19:02:38 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

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

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

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

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

Andres Freund <andres(at)anarazel(dot)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

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

Hi,

On 2016-08-22 16:20:58 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

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

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

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

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

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

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

On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres(at)anarazel(dot)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

On Sun, Aug 28, 2016 at 3:18 AM, Andres Freund <andres(at)anarazel(dot)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

On Fri, Sep 2, 2016 at 3:31 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres(at)anarazel(dot)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

On 2016-09-02 09:05:35 -0500, Kevin Grittner wrote:
> On Fri, Sep 2, 2016 at 3:31 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres(at)anarazel(dot)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

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

On Fri, Sep 2, 2016 at 9:11 AM, Andres Freund <andres(at)anarazel(dot)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(at)gmail(dot)com> wrote:
>>> On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres(at)anarazel(dot)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

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(at)gmail(dot)com> wrote:
> > > On Tue, Aug 23, 2016 at 3:10 AM, Andres Freund <andres(at)anarazel(dot)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).

Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

On 2016-09-02 10:20:42 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

On Fri, Sep 2, 2016 at 9:25 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)anarazel(dot)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

Kevin Grittner <kgrittn(at)gmail(dot)com> writes:
> On Fri, Sep 2, 2016 at 9:25 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Fri, Sep 2, 2016 at 9:51 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Kevin Grittner <kgrittn(at)gmail(dot)com> writes:
> On Fri, Sep 2, 2016 at 9:51 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On 2016-09-02 10:34:54 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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).

On 2016-09-02 09:41:28 -0500, Kevin Grittner wrote:
> On Fri, Sep 2, 2016 at 9:25 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Andres Freund <andres(at)anarazel(dot)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

On Fri, Sep 2, 2016 at 10:31 AM, Andres Freund <andres(at)anarazel(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> > Andres Freund <andres(at)anarazel(dot)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

On Fri, Sep 2, 2016 at 10:10 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Fri, Sep 2, 2016 at 10:10 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)gmail(dot)com> writes:
>> On Fri, Sep 2, 2016 at 9:51 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

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

Andres Freund <andres(at)anarazel(dot)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

On 2016-09-02 14:04:24 +0530, Robert Haas wrote:
> On Sun, Aug 28, 2016 at 3:18 AM, Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

On 2016-09-12 10:19:14 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

On 2016-09-12 11:29:37 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

Hi,

On 2016-09-12 12:10:01 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)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

Hi,

On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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.

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
>> Andres Freund <andres(at)anarazel(dot)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

On 2016-09-12 13:48:05 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
> >> Andres Freund <andres(at)anarazel(dot)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

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2016-09-12 13:48:05 -0400, Tom Lane wrote:
>> Andres Freund <andres(at)anarazel(dot)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

On 2016-09-12 14:05:33 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2016-09-12 13:48:05 -0400, Tom Lane wrote:
> >> Andres Freund <andres(at)anarazel(dot)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.

Andres Freund <andres(at)anarazel(dot)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$(at)pcorp(dot)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

On 2016-09-12 17:36:07 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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$(at)pcorp(dot)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

Andres Freund <andres(at)anarazel(dot)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$(at)pcorp(dot)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

Hi,

On 2016-09-12 18:35:03 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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