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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 23 May 2016 at 08:53, Andres Freund <andres@anarazel.de> wrote:
Hi,
discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in the target
list.One idea I circulated was to fix that by interjecting a special executor
node to process SRF containing targetlists (reusing Result possibly?).
That'd allow to remove the isDone argument from ExecEval*/ExecProject*
and get rid of ps_TupFromTlist which is fairly ugly.Robert suggested - IIRC mentioning previous on-list discussion - to
instead rewrite targetlist SRFs into lateral joins. My gut feeling is
that that'd be a larger undertaking, with significant semantics changes.If we accept bigger semantical changes, I'm inclined to instead just get
rid of targetlist SRFs in total; they're really weird and not needed
anymore.One issue with removing targetlist SRFs is that they're currently
considerably faster than SRFs in FROM:
tpch[14693][1]=# COPY (SELECT * FROM generate_series(1, 10000000)) TO
'/dev/null';
COPY 10000000
Time: 2217.167 ms
tpch[14693][1]=# COPY (SELECT generate_series(1, 10000000)) TO '/dev/null';
COPY 10000000
Time: 1355.929 ms
tpch[14693][1]=#I'm no tto concerned about that, and we could probably fixing by
removing forced materialization from the relevant code path.Comments?
SRFs-in-tlist are a lot faster for lockstep iteration etc. They're also
much simpler to write, though if the result result rowcount differs
unexpectedly between the functions you get exciting and unexpected
behaviour.
WITH ORDINALITY provides what I think is the last of the functionality
needed to replace SRFs-in-from, but at a syntatactic complexity and
performance cost. The following example demonstrates that, though it
doesn't do anything that needs LATERAL etc. I'm aware the following aren't
semantically identical if the rowcounts differ.
craig=> EXPLAIN ANALYZE SELECT generate_series(1,1000000) x,
generate_series(1,1000000) y;
QUERY PLAN
----------------------------------------------------------------------------------------------
Result (cost=0.00..5.01 rows=1000 width=0) (actual time=0.024..92.845
rows=1000000 loops=1)
Planning time: 0.039 ms
Execution time: 123.123 ms
(3 rows)
Time: 123.719 ms
craig=> EXPLAIN ANALYZE SELECT x, y FROM generate_series(1,1000000) WITH
ORDINALITY AS x(i, n) INNER JOIN generate_series(1,1000000) WITH ORDINALITY
AS y(i, n) ON (x.n = y.n);
QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------
Merge Join (cost=0.01..97.50 rows=5000 width=64) (actual
time=179.863..938.375 rows=1000000 loops=1)
Merge Cond: (x.n = y.n)
-> Function Scan on generate_series x (cost=0.00..10.00 rows=1000
width=40) (actual time=108.813..303.690 rows=1000000 loops=1)
-> Materialize (cost=0.00..12.50 rows=1000 width=40) (actual
time=71.043..372.880 rows=1000000 loops=1)
-> Function Scan on generate_series y (cost=0.00..10.00
rows=1000 width=40) (actual time=71.039..266.209 rows=1000000 loops=1)
Planning time: 0.184 ms
Execution time: 970.744 ms
(7 rows)
Time: 971.706 ms
I get the impression the with-ordinality case could perform just as well if
the optimiser recognised a join on the ordinality column and iterated the
functions in lockstep to populate the result row directly. Though that
could perform _worse_ if the function is computationally costly and
benefits significantly from the CPU cache, where we're better off
materializing it or at least executing it in chunks/batches...
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
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.
/messages/by-id/25750.1458767514@sss.pgh.pa.us
On Sun, May 22, 2016 at 8:53 PM, Andres Freund <andres@anarazel.de> wrote:
Hi,
discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in the target
list.One idea I circulated was to fix that by interjecting a special executor
node to process SRF containing targetlists (reusing Result possibly?).
That'd allow to remove the isDone argument from ExecEval*/ExecProject*
and get rid of ps_TupFromTlist which is fairly ugly.
Conceptually I'm all for minimizing the impact on queries of this form.
It seems to be the most likely to get written and committed and the least
likely to cause unforeseen issues.
Robert suggested - IIRC mentioning previous on-list discussion - to
instead rewrite targetlist SRFs into lateral joins. My gut feeling is
that that'd be a larger undertaking, with significant semantics changes.
[...]
If we accept bigger semantical changes, I'm inclined to instead just get
rid of targetlist SRFs in total; they're really weird and not needed
anymore.
I cannot see these, in isolation, being a good option. Nonetheless, I
don't think any semantic change should happen before 9.2 becomes no longer
supported. I'd be inclined to take a similar approach as with
standard_conforming_strings (minus the execution guc, just the warning one)
with whatever after-the-fact learning taken into account.
Its worth considering query rewrite and making it forbidden as a joint goal.
For something like a canonical version of this, especially for
composite-returning SRF:
WITH func_call (
SELECT func(tbl.col)
FROM tbl
)
SELECT (func_call.func).*
FROM func_call;
If we can rewrite the CTE portion into a lateral - with the exact same
semantics (specifically, returning the single-column composite) then check
the rewritten query the select list SRF would not longer be present and no
error would be thrown.
For situations where a rewrite cannot be made to behave properly we leave
the construct alone and let the query raise an error.
In considering what I just wrote I'm not particularly enamored with
it...hence my overall conclusion. Can't say I hate it and after re-reading
the aforementioned thread I'm inclined to like it for cases where, for
instance, we are susceptible to a LCM evaluation.
David J.
Andres Freund <andres@anarazel.de> writes:
discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in the target
list.
Yeah, this has been an annoyance for a long time.
One idea I circulated was to fix that by interjecting a special executor
node to process SRF containing targetlists (reusing Result possibly?).
That'd allow to remove the isDone argument from ExecEval*/ExecProject*
and get rid of ps_TupFromTlist which is fairly ugly.
Would that not lead to, in effect, duplicating all of execQual.c? The new
executor node would still have to be prepared to process all expression
node types.
Robert suggested - IIRC mentioning previous on-list discussion - to
instead rewrite targetlist SRFs into lateral joins. My gut feeling is
that that'd be a larger undertaking, with significant semantics changes.
Yes, this was discussed on-list awhile back (I see David found a reference
already). I think it's feasible, although we'd first have to agree
whether we want to remain bug-compatible with the old
least-common-multiple-of-the-periods behavior. I would vote for not,
but it's certainly a debatable thing.
If we accept bigger semantical changes, I'm inclined to instead just get
rid of targetlist SRFs in total; they're really weird and not needed
anymore.
This seems a bridge too far to me. It's just way too common to do
"select generate_series(1,n)". We could tell people they have to
rewrite to "select * from generate_series(1,n)", but it would be far
more polite to do that for them.
One issue with removing targetlist SRFs is that they're currently
considerably faster than SRFs in FROM:
I suspect that depends greatly on your test case. But in any case
we could put more effort into optimizing nodeFunctionscan.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote:
This seems a bridge too far to me. It's just way too common to do
"select generate_series(1,n)". We could tell people they have to
rewrite to "select * from generate_series(1,n)", but it would be far
more polite to do that for them.
How about making "TABLE generate_series(1,n)" work? It's even
shorter in exchange for some cognitive load.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter <david@fetter.org> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote:
This seems a bridge too far to me. It's just way too common to do
"select generate_series(1,n)". We could tell people they have to
rewrite to "select * from generate_series(1,n)", but it would be far
more polite to do that for them.How about making "TABLE generate_series(1,n)" work? It's even
shorter in exchange for some cognitive load.No thanks --- the word after TABLE ought to be a table name, not some
arbitrary expression. That's way too much mess to save one keystroke.
It's not just about saving a keystroke. This change would go with
removing the ability to do SRFs in the target list of a SELECT
query.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
David Fetter <david@fetter.org> writes:
On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
How about making "TABLE generate_series(1,n)" work? It's even
shorter in exchange for some cognitive load.
No thanks --- the word after TABLE ought to be a table name, not some
arbitrary expression. That's way too much mess to save one keystroke.
It's not just about saving a keystroke. This change would go with
removing the ability to do SRFs in the target list of a SELECT
query.
I guess you did not understand that I was rejecting doing that.
Telling people they have to modify existing code that does this and
works fine is exactly what I felt we can't do. We might be able to
blow off complicated cases, but I think simpler cases are too common
in the field.
I'm on board with fixing things so that the *implementation* doesn't
support SRF-in-tlist. But we can't just remove it from the language.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in the target
list.Yeah, this has been an annoyance for a long time.
One idea I circulated was to fix that by interjecting a special executor
node to process SRF containing targetlists (reusing Result possibly?).
That'd allow to remove the isDone argument from ExecEval*/ExecProject*
and get rid of ps_TupFromTlist which is fairly ugly.Would that not lead to, in effect, duplicating all of execQual.c? The new
executor node would still have to be prepared to process all expression
node types.Robert suggested - IIRC mentioning previous on-list discussion - to
instead rewrite targetlist SRFs into lateral joins. My gut feeling is
that that'd be a larger undertaking, with significant semantics changes.Yes, this was discussed on-list awhile back (I see David found a reference
already). I think it's feasible, although we'd first have to agree
whether we want to remain bug-compatible with the old
least-common-multiple-of-the-periods behavior. I would vote for not,
but it's certainly a debatable thing.
+1 on removing LCM.
The behavior of multiple targetlist SRF is so bizarre that it's
incredible to believe anyone would reasonably expect it to work that
way. Agree also that casual sane usage of target list SRF is
incredibly common via generate_series() and unnest() etc is
exceptionally common...better not to break those cases without a
better justification than code simplicity.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 1:44 PM, David Fetter <david@fetter.org> wrote:
On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote:
David Fetter <david@fetter.org> writes:
On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote:
This seems a bridge too far to me. It's just way too common to do
"select generate_series(1,n)". We could tell people they have to
rewrite to "select * from generate_series(1,n)", but it would be far
more polite to do that for them.How about making "TABLE generate_series(1,n)" work? It's even
shorter in exchange for some cognitive load.No thanks --- the word after TABLE ought to be a table name, not some
arbitrary expression. That's way too much mess to save one keystroke.It's not just about saving a keystroke. This change would go with
removing the ability to do SRFs in the target list of a SELECT
query.
If you want to make an argument for doing this regardless of the target
list SRF change by all means - but it does absolutely nothing to mitigate
the breakage that would result if we choose this path.
David J.
On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in the target
list.Yeah, this has been an annoyance for a long time.
One idea I circulated was to fix that by interjecting a special executor
node to process SRF containing targetlists (reusing Result possibly?).
That'd allow to remove the isDone argument from ExecEval*/ExecProject*
and get rid of ps_TupFromTlist which is fairly ugly.Would that not lead to, in effect, duplicating all of execQual.c? The new
executor node would still have to be prepared to process all expression
node types.Robert suggested - IIRC mentioning previous on-list discussion - to
instead rewrite targetlist SRFs into lateral joins. My gut feeling is
that that'd be a larger undertaking, with significant semantics changes.Yes, this was discussed on-list awhile back (I see David found a reference
already). I think it's feasible, although we'd first have to agree
whether we want to remain bug-compatible with the old
least-common-multiple-of-the-periods behavior. I would vote for not,
but it's certainly a debatable thing.+1 on removing LCM.
As a green field project, that would make total sense. As a thing
decades in, it's not clear to me that that would break less stuff or
break it worse than simply disallowing SRFs in the target list, which
has been rejected on bugward-compatibility grounds. I suspect it
would be even worse because disallowing SRFs in target lists would at
least be obvious and localized when it broke code.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in the target
list.Yeah, this has been an annoyance for a long time.
One idea I circulated was to fix that by interjecting a special executor
node to process SRF containing targetlists (reusing Result possibly?).
That'd allow to remove the isDone argument from ExecEval*/ExecProject*
and get rid of ps_TupFromTlist which is fairly ugly.Would that not lead to, in effect, duplicating all of execQual.c? The new
executor node would still have to be prepared to process all expression
node types.Robert suggested - IIRC mentioning previous on-list discussion - to
instead rewrite targetlist SRFs into lateral joins. My gut feeling is
that that'd be a larger undertaking, with significant semantics changes.Yes, this was discussed on-list awhile back (I see David found a reference
already). I think it's feasible, although we'd first have to agree
whether we want to remain bug-compatible with the old
least-common-multiple-of-the-periods behavior. I would vote for not,
but it's certainly a debatable thing.+1 on removing LCM.
As a green field project, that would make total sense. As a thing
decades in, it's not clear to me that that would break less stuff or
break it worse than simply disallowing SRFs in the target list, which
has been rejected on bugward-compatibility grounds. I suspect it
would be even worse because disallowing SRFs in target lists would at
least be obvious and localized when it broke code.
If I'm reading this correctly, it sounds to me like you are making the
case that removing target list SRF completely would somehow cause less
breakage than say, rewriting it to a LATERAL based implementation for
example. With more than a little forbearance, let's just say I don't
agree.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/23/2016 12:39 PM, Merlin Moncure wrote:
On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in the target
list.Yeah, this has been an annoyance for a long time.
One idea I circulated was to fix that by interjecting a special executor
node to process SRF containing targetlists (reusing Result possibly?).
That'd allow to remove the isDone argument from ExecEval*/ExecProject*
and get rid of ps_TupFromTlist which is fairly ugly.Would that not lead to, in effect, duplicating all of execQual.c? The new
executor node would still have to be prepared to process all expression
node types.Robert suggested - IIRC mentioning previous on-list discussion - to
instead rewrite targetlist SRFs into lateral joins. My gut feeling is
that that'd be a larger undertaking, with significant semantics changes.Yes, this was discussed on-list awhile back (I see David found a reference
already). I think it's feasible, although we'd first have to agree
whether we want to remain bug-compatible with the old
least-common-multiple-of-the-periods behavior. I would vote for not,
but it's certainly a debatable thing.+1 on removing LCM.
As a green field project, that would make total sense. As a thing
decades in, it's not clear to me that that would break less stuff or
break it worse than simply disallowing SRFs in the target list, which
has been rejected on bugward-compatibility grounds. I suspect it
would be even worse because disallowing SRFs in target lists would at
least be obvious and localized when it broke code.If I'm reading this correctly, it sounds to me like you are making the
case that removing target list SRF completely would somehow cause less
breakage than say, rewriting it to a LATERAL based implementation for
example. With more than a little forbearance, let's just say I don't
agree.
I'm not necessarily saying that we should totally remove target list
SRFs, but I will point out it has been deprecated ever since SRFs were
first introduced:
http://www.postgresql.org/docs/7.3/static/xfunc-sql.html
"Currently, functions returning sets may also be called in the target
list of a SELECT query. For each row that the SELECT generates by
itself, the function returning set is invoked, and an output row is
generated for each element of the function's result set. Note,
however, that this capability is deprecated and may be removed in
future releases."
I would be in favor of rewriting it to a LATERAL, but that would not be
backwards compatible entirely either IIUC.
I'll also note that, unless I missed something, we also have to consider
that the capability to pipeline results is still only available in the
target list.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
On Mon, May 23, 2016 at 02:39:54PM -0500, Merlin Moncure wrote:
On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in the target
list.Yeah, this has been an annoyance for a long time.
One idea I circulated was to fix that by interjecting a special executor
node to process SRF containing targetlists (reusing Result possibly?).
That'd allow to remove the isDone argument from ExecEval*/ExecProject*
and get rid of ps_TupFromTlist which is fairly ugly.Would that not lead to, in effect, duplicating all of execQual.c? The new
executor node would still have to be prepared to process all expression
node types.Robert suggested - IIRC mentioning previous on-list discussion - to
instead rewrite targetlist SRFs into lateral joins. My gut feeling is
that that'd be a larger undertaking, with significant semantics changes.Yes, this was discussed on-list awhile back (I see David found a reference
already). I think it's feasible, although we'd first have to agree
whether we want to remain bug-compatible with the old
least-common-multiple-of-the-periods behavior. I would vote for not,
but it's certainly a debatable thing.+1 on removing LCM.
As a green field project, that would make total sense. As a thing
decades in, it's not clear to me that that would break less stuff or
break it worse than simply disallowing SRFs in the target list, which
has been rejected on bugward-compatibility grounds. I suspect it
would be even worse because disallowing SRFs in target lists would at
least be obvious and localized when it broke code.If I'm reading this correctly, it sounds to me like you are making the
case that removing target list SRF completely would somehow cause less
breakage than say, rewriting it to a LATERAL based implementation for
example.
Yes.
Making SRFs in target lists throw an error is a thing that will be
pretty straightforward to deal with in extant code bases, whatever
size of pain in the neck it might be. The line of code that caused
the error would be very clear, and the fix would be very obvious.
Making their behavior different in some way that throws no warnings is
guaranteed to cause subtle and hard to track bugs in extant code
bases. We lost not a few existing users when we caused similar
knock-ons in 8.3 by removing automated casts to text.
I am no longer advocating for removing the functionality. I am just
pointing out that the knock-on effects of changing the functionality
may well cause more pain than the ones from removing it entirely.
With more than a little forbearance, let's just say I don't agree.
If you'd be so kind as to explain your reasons, I think we'd all
benefit.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Merlin Moncure <mmoncure@gmail.com> writes:
On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
+1 on removing LCM.
As a green field project, that would make total sense. As a thing
decades in, it's not clear to me that that would break less stuff or
break it worse than simply disallowing SRFs in the target list, which
has been rejected on bugward-compatibility grounds. I suspect it
would be even worse because disallowing SRFs in target lists would at
least be obvious and localized when it broke code.
If I'm reading this correctly, it sounds to me like you are making the
case that removing target list SRF completely would somehow cause less
breakage than say, rewriting it to a LATERAL based implementation for
example. With more than a little forbearance, let's just say I don't
agree.
We should consider single and multiple SRFs in a targetlist as distinct
use-cases; only the latter has got weird properties.
There are several things we could potentially do with multiple SRFs in
the same targetlist. In increasing order of backwards compatibility and
effort required:
1. Throw error if there's more than one SRF.
2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would
have the same behavior as before if the SRFs all return the same number
of rows, and otherwise would behave differently.
3. Rewrite into some other construct that still ends up as a FunctionScan
RTE node, but has the old LCM behavior if the SRFs produce different
numbers of rows. (Perhaps we would not need to expose this construct
as something directly SQL-visible.)
It's certainly arguable that the common use-cases for SRF-in-tlist
don't have more than one SRF per tlist, and thus that implementing #1
would be an appropriate amount of effort. It's worth noting also that
the LCM behavior has been repeatedly reported as a bug, and therefore
that if we do #3 we'll be expending very substantial effort to be
literally bug-compatible with ancient behavior that no one in the
current development group thinks is well-designed. As far as #2 goes,
it would have the advantage that code depending on the same-number-of-
rows case would continue to work as before. David has a point that it
would silently break application code that's actually depending on the
LCM behavior, but how much of that is there likely to be, really?
[ reflects a bit... ] I guess there is room for an option 2-and-a-half:
2.5. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...), but decorate
the FunctionScan RTE to tell the executor to throw an error if the SRFs
don't all return the same number of rows, rather than silently
null-padding. This would have the same behavior as before for the sane
case, and would be very not-silent about cases where apps actually invoked
the LCM behavior. Again, we wouldn't necessarily have to expose such an
option at the SQL level. (Though it strikes me that such a restriction
could have value in its own right, analogous to the STRICT options that
we've invented in some other places to allow insisting on the expected
numbers of rows being returned. ROWS FROM STRICT (srf1(), srf2(), ...),
anybody?)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 4:05 PM, David Fetter <david@fetter.org> wrote:
On Mon, May 23, 2016 at 02:39:54PM -0500, Merlin Moncure wrote:
On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
On Mon, May 23, 2016 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
discussing executor performance with a number of people at pgcon,
several hackers - me included - complained about the additional
complexity, both code and runtime, required to handle SRFs in thetarget
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 feelingis
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 fornot,
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@joeconway.com> writes:
I would be in favor of rewriting it to a LATERAL, but that would not be
backwards compatible entirely either IIUC.
It could be made so, I think, but it may be more trouble than it's worth;
see my previous message.
I'll also note that, unless I missed something, we also have to consider
that the capability to pipeline results is still only available in the
target list.
Yes, we would definitely want to improve nodeFunctionscan.c to perform
better for ValuePerCall SRFs. But that has value independently of this.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Tom Lane wrote:
Joe Conway <mail@joeconway.com> writes:
I'll also note that, unless I missed something, we also have to consider
that the capability to pipeline results is still only available in the
target list.Yes, we would definitely want to improve nodeFunctionscan.c to perform
better for ValuePerCall SRFs. But that has value independently of this.
Ah, so that's what "pipeline results" mean! I hadn't gotten that. I
agree; Abhijit had a patch or a plan for this, a long time ago ...
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, May 23, 2016 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Merlin Moncure <mmoncure@gmail.com> writes:
On Mon, May 23, 2016 at 2:13 PM, David Fetter <david@fetter.org> wrote:
On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote:
+1 on removing LCM.
As a green field project, that would make total sense. As a thing
decades in, it's not clear to me that that would break less stuff or
break it worse than simply disallowing SRFs in the target list, which
has been rejected on bugward-compatibility grounds. I suspect it
would be even worse because disallowing SRFs in target lists would at
least be obvious and localized when it broke code.If I'm reading this correctly, it sounds to me like you are making the
case that removing target list SRF completely would somehow cause less
breakage than say, rewriting it to a LATERAL based implementation for
example. With more than a little forbearance, let's just say I don't
agree.We should consider single and multiple SRFs in a targetlist as distinct
use-cases; only the latter has got weird properties.There are several things we could potentially do with multiple SRFs in
the same targetlist. In increasing order of backwards compatibility and
effort required:1. Throw error if there's more than one SRF.
2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would
have the same behavior as before if the SRFs all return the same number
of rows, and otherwise would behave differently.3. Rewrite into some other construct that still ends up as a FunctionScan
RTE node, but has the old LCM behavior if the SRFs produce different
numbers of rows. (Perhaps we would not need to expose this construct
as something directly SQL-visible.)It's certainly arguable that the common use-cases for SRF-in-tlist
don't have more than one SRF per tlist, and thus that implementing #1
would be an appropriate amount of effort. It's worth noting also that
the LCM behavior has been repeatedly reported as a bug, and therefore
that if we do #3 we'll be expending very substantial effort to be
literally bug-compatible with ancient behavior that no one in the
current development group thinks is well-designed. As far as #2 goes,
it would have the advantage that code depending on the same-number-of-
rows case would continue to work as before. David has a point that it
would silently break application code that's actually depending on the
LCM behavior, but how much of that is there likely to be, really?[ reflects a bit... ] I guess there is room for an option 2-and-a-half:
2.5. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...), but decorate
the FunctionScan RTE to tell the executor to throw an error if the SRFs
don't all return the same number of rows, rather than silently
null-padding. This would have the same behavior as before for the sane
case, and would be very not-silent about cases where apps actually invoked
the LCM behavior. Again, we wouldn't necessarily have to expose such an
option at the SQL level. (Though it strikes me that such a restriction
could have value in its own right, analogous to the STRICT options that
we've invented in some other places to allow insisting on the expected
numbers of rows being returned. ROWS FROM STRICT (srf1(), srf2(), ...),
anybody?)
I'd let the engineers decide between 1, 2.5, and 3 - but if we accomplish
our goals while implementing #3 I'd say that would be the best outcome for
the community as whole.
We don't have the luxury of providing a safe-usage mode where people
writing new queries get the error but pre-existing queries are considered
OK. We will have to rely upon education and deal with the occasional bug
report but our long-time customers, even if only a minority would be
affected, will appreciate the effort taken to not break code that has been
working for a long time.
The minority is likely small enough to at least make options 1 and 2.5
viable though I'd think we make an effort to avoid #1.
David J.
On Mon, May 23, 2016 at 4:24 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
Tom Lane wrote:
Joe Conway <mail@joeconway.com> writes:
I'll also note that, unless I missed something, we also have to
consider
that the capability to pipeline results is still only available in the
target list.Yes, we would definitely want to improve nodeFunctionscan.c to perform
better for ValuePerCall SRFs. But that has value independently of this.Ah, so that's what "pipeline results" mean! I hadn't gotten that. I
agree; Abhijit had a patch or a plan for this, a long time ago ...
Is this sidebar strictly an implementation detail, not user visible?
David J.
