Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f)

On Fri, Jan 27, 2017 at 5:28 AM, Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

Consider the below test;

CREATE TABLE tab ( a int primary key);

SELECT *
FROM pg_constraint pc,
CAST(CASE WHEN pc.contype IN ('f','u','p') THEN generate_series(1,
array_upper(pc.conkey, 1)) ELSE NULL END AS int) AS position;

Above query is failing with "set-valued function called in context that
cannot
accept a set". But if I remove the CASE from the query then it working
just good.

Like:

SELECT *
FROM pg_constraint pc,
CAST(generate_series(1, array_upper(pc.conkey, 1)) AS int) AS position;

This started failing with 69f4b9c85f168ae006929eec44fc44d569e846b9. It
seems
check_srf_call_placement() sets the hasTargetSRFs flag and but when the
SRFs
at the rtable ofcourse this flag doesn't get set. It seems like missing
something
their, but I might be completely wrong as not quire aware of this area.

I'm a bit surprised that your query actually works...and without delving
into source code its hard to explain why it should/shouldn't or whether the
recent SRF work was intended to impact it.

In any case the more idiomatic way of writing your query these days (since
9.4 came out) is:

SELECT *
FROM pg_constraint pc
LEFT JOIN LATERAL generate_series(1, case when contype in ('f','p','u')
then array_upper(pc.conkey, 1) else 0 end) gs ON true;

generate_series is smart enough to return an empty set (instead of erroring
out) when provided with (1,0) as arguments.

David J.

On Fri, Jan 27, 2017 at 3:13 PM, David G. Johnston <
david.g.johnston@gmail.com> wrote:

In any case the more idiomatic way of writing your query these days (since
9.4 came out) is:

SELECT *
FROM pg_constraint pc
LEFT JOIN LATERAL generate_series(1, case when contype in ('f','p','u')
then array_upper(pc.conkey, 1) else 0 end) gs ON true;

Supposedly ​should work back to 9.3​, mis-remembered when LATERAL was
released.

David J.

On Sat, Jan 28, 2017 at 3:43 AM, David G. Johnston <
david.g.johnston@gmail.com> wrote:

On Fri, Jan 27, 2017 at 5:28 AM, Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

Consider the below test;

CREATE TABLE tab ( a int primary key);

SELECT *
FROM pg_constraint pc,
CAST(CASE WHEN pc.contype IN ('f','u','p') THEN generate_series(1,
array_upper(pc.conkey, 1)) ELSE NULL END AS int) AS position;

Above query is failing with "set-valued function called in context that
cannot
accept a set". But if I remove the CASE from the query then it working
just good.

Like:

SELECT *
FROM pg_constraint pc,
CAST(generate_series(1, array_upper(pc.conkey, 1)) AS int) AS position;

This started failing with 69f4b9c85f168ae006929eec44fc44d569e846b9. It
seems
check_srf_call_placement() sets the hasTargetSRFs flag and but when the
SRFs
at the rtable ofcourse this flag doesn't get set. It seems like missing
something
their, but I might be completely wrong as not quire aware of this area.

I'm a bit surprised that your query actually works...and without delving
into source code its hard to explain why it should/shouldn't or whether the
recent SRF work was intended to impact it.

In any case the more idiomatic way of writing your query these days (since
9.4 came out) is:

SELECT *
FROM pg_constraint pc
LEFT JOIN LATERAL generate_series(1, case when contype in ('f','p','u')
then array_upper(pc.conkey, 1) else 0 end) gs ON true;

generate_series is smart enough to return an empty set (instead of
erroring out) when provided with (1,0) as arguments.

Thanks for the providing work-around query and I also understood your point.

At the same time reason to raise this issue was, because this was working
before
69f4b9c85f168ae006929eec44fc44d569e846b9 commit and now its throwing
an error. So whether its intended or query started failing because of some
bug introduced with the commit.

Issues is reproducible when query re-written with LEFT JOIN LATERAL and I
continue to use CASE statement.

SELECT *
FROM pg_constraint pc
LEFT JOIN LATERAL CAST((CASE WHEN pc.contype IN ('f','u','p') THEN
generate_series(1, array_upper(pc.conkey, 1)) ELSE NULL END) AS int) gs ON
true;
ERROR: set-valued function called in context that cannot accept a set

David J.

--
Rushabh Lathia
www.EnterpriseDB.com

Hi,

On 2017-01-27 17:58:04 +0530, Rushabh Lathia wrote:

Consider the below test;

CREATE TABLE tab ( a int primary key);

SELECT *
FROM pg_constraint pc,
CAST(CASE WHEN pc.contype IN ('f','u','p') THEN generate_series(1,
array_upper(pc.conkey, 1)) ELSE NULL END AS int) AS position;

Above query is failing with "set-valued function called in context that
cannot
accept a set".

I think that's correct. Functions in FROM are essentially a shorthand
for ROWS FROM(). And ROWS FROM doesn't allow arbitrary expressions. It
works if you remove the CASE because then it's a valid ROWS FROM
content.

If, I didn't check, that worked previously, I think that was more
accident than intent.

But if I remove the CASE from the query then it working just
good.

Like:

SELECT *
FROM pg_constraint pc,
CAST(generate_series(1, array_upper(pc.conkey, 1)) AS int) AS position;

This IMO shouldn't work either due to the CAST. But indeed it does.

This started failing with 69f4b9c85f168ae006929eec44fc44d569e846b9. It seems
check_srf_call_placement() sets the hasTargetSRFs flag and but when the SRFs
at the rtable ofcourse this flag doesn't get set. It seems like missing
something
their, but I might be completely wrong as not quire aware of this area.

That's right, because it's not in the targetlist.

Regards,

Andres

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

Andres Freund <andres@anarazel.de> writes:

On 2017-01-27 17:58:04 +0530, Rushabh Lathia wrote:

SELECT *
FROM pg_constraint pc,
CAST(CASE WHEN pc.contype IN ('f','u','p') THEN generate_series(1,
array_upper(pc.conkey, 1)) ELSE NULL END AS int) AS position;

Above query is failing with "set-valued function called in context that
cannot accept a set".

I think that's correct. Functions in FROM are essentially a shorthand
for ROWS FROM(). And ROWS FROM doesn't allow arbitrary expressions.

No, but it allows whatever looks syntactically like a function, including
casts. IIRC, we made func_expr work that way ages ago to deflect
complaints that it wasn't very clear why some things-that-look-like-
functions were allowed in CREATE INDEX and others not.

If, I didn't check, that worked previously, I think that was more
accident than intent.

Yeah, probably. But are we prepared to break working queries?
As I understood it, the agreement on this whole tlist-SRF change
was that we would not change any behavior that wasn't ill-defined.

We could probably fix this with the modification that was discussed
previously, to allow FunctionScan nodes to project a scalar tlist
from the outputs of their SRFs.

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

Hi,

On 2017-01-30 16:55:56 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2017-01-27 17:58:04 +0530, Rushabh Lathia wrote:

SELECT *
FROM pg_constraint pc,
CAST(CASE WHEN pc.contype IN ('f','u','p') THEN generate_series(1,
array_upper(pc.conkey, 1)) ELSE NULL END AS int) AS position;

Above query is failing with "set-valued function called in context that
cannot accept a set".

I think that's correct. Functions in FROM are essentially a shorthand
for ROWS FROM(). And ROWS FROM doesn't allow arbitrary expressions.

No, but it allows whatever looks syntactically like a function, including
casts. IIRC, we made func_expr work that way ages ago to deflect
complaints that it wasn't very clear why some things-that-look-like-
functions were allowed in CREATE INDEX and others not.

But given e.g. the above example that's just about no limitation at all,
because you can nest nearly arbitrarily complex things within the
expression.

If, I didn't check, that worked previously, I think that was more
accident than intent.

Yeah, probably.

Really looks that way. I think it only works that way because we hit the
recovery branch for:
/*
* Normally the passed expression tree will be a FuncExprState, since the
* grammar only allows a function call at the top level of a table
* function reference. However, if the function doesn't return set then
* the planner might have replaced the function call via constant-folding
* or inlining. So if we see any other kind of expression node, execute
* it via the general ExecEvalExpr() code; the only difference is that we
* don't get a chance to pass a special ReturnSetInfo to any functions
* buried in the expression.
*/
which does a normal ExecEvalExpr() whenever the expression to be
evaluated isn't a FuncExpr.

At the very least I think we need to amend that paragraph explaining
that there's a bunch of other cases it can be hit. And add tests for it.

But are we prepared to break working queries?

Within some limits, we imo should be.

As I understood it, the agreement on this whole tlist-SRF change
was that we would not change any behavior that wasn't ill-defined.

I'd argue that behaviour that only worked through some edge case is
kinda ill defined ;) (and no, I'm not that serious)

We could probably fix this with the modification that was discussed
previously, to allow FunctionScan nodes to project a scalar tlist
from the outputs of their SRFs.

Hm. I'm not quite following. Could you expand?

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

Andres Freund <andres@anarazel.de> writes:

On 2017-01-30 16:55:56 -0500, Tom Lane wrote:

No, but it allows whatever looks syntactically like a function, including
casts. IIRC, we made func_expr work that way ages ago to deflect
complaints that it wasn't very clear why some things-that-look-like-
functions were allowed in CREATE INDEX and others not.

But given e.g. the above example that's just about no limitation at all,
because you can nest nearly arbitrarily complex things within the
expression.

Yeah, exactly. But that's true anyway because even if it was
syntactically a plain function, it might've been a SQL function that the
planner chooses to inline.

But are we prepared to break working queries?

Within some limits, we imo should be.

In this case I think the argument for rejecting is pretty darn weak;
it's an arbitrary implementation restriction, not anything with much
principle to it.

We could probably fix this with the modification that was discussed
previously, to allow FunctionScan nodes to project a scalar tlist
from the outputs of their SRFs.

Hm. I'm not quite following. Could you expand?

Make it work like Agg and WindowFunc. To wit, dump the actually special
function calls (the set-returning functions) into a list that's internal
to the FunctionScan node, and then anything above those goes into scalar
expressions in the node's tlist, which refer to the SRF outputs using
Vars or things morally equivalent to Vars.

This would not support nested SRFs in FROM, but that case has always
failed and I've heard no field requests to make it work, so I don't feel
bad about keeping that restriction.

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 2017-01-30 17:24:31 -0500, Tom Lane wrote:

Make it work like Agg and WindowFunc. To wit, dump the actually special
function calls (the set-returning functions) into a list that's internal
to the FunctionScan node, and then anything above those goes into scalar
expressions in the node's tlist, which refer to the SRF outputs using
Vars or things morally equivalent to Vars.

Hm. That should be fairly doable. (I'd advocate very strongly against
building that list via ExecInitExpr, but that's an implementation
detail). We'd evaluate SRFs early, but that's just consistent with
targetlist SRFs.

Wonder if we there's an argument to be made for implementing this
roughly similarly to split_pathtarget_at_srf - instead of injecting a
ProjectSet node we'd add a FunctionScan node below a Result node.

- Andres

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

Andres Freund <andres@anarazel.de> writes:

Wonder if we there's an argument to be made for implementing this
roughly similarly to split_pathtarget_at_srf - instead of injecting a
ProjectSet node we'd add a FunctionScan node below a Result node.

Yeah, possibly. That would have the advantage of avoiding an ExecProject
step when the SRFs aren't buried, which would certainly be the expected
case.

If you don't want to make ExecInitExpr responsible, then the planner would
have to do something like split_pathtarget_at_srf anyway to decompose the
expressions, no matter which executor representation we use.

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, Jan 30, 2017 at 6:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

Wonder if we there's an argument to be made for implementing this
roughly similarly to split_pathtarget_at_srf - instead of injecting a
ProjectSet node we'd add a FunctionScan node below a Result node.

Yeah, possibly. That would have the advantage of avoiding an ExecProject
step when the SRFs aren't buried, which would certainly be the expected
case.

If you don't want to make ExecInitExpr responsible, then the planner would
have to do something like split_pathtarget_at_srf anyway to decompose the
expressions, no matter which executor representation we use.

Did we do anything about this? Are we going to?

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

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

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jan 30, 2017 at 6:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

If you don't want to make ExecInitExpr responsible, then the planner would
have to do something like split_pathtarget_at_srf anyway to decompose the
expressions, no matter which executor representation we use.

Did we do anything about this? Are we going to?

No, and I think we should. Is it on the v10 open items list?

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, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Jan 30, 2017 at 6:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

If you don't want to make ExecInitExpr responsible, then the planner would
have to do something like split_pathtarget_at_srf anyway to decompose the
expressions, no matter which executor representation we use.

Did we do anything about this? Are we going to?

No, and I think we should. Is it on the v10 open items list?

Wasn't, I've added it now:

https://wiki.postgresql.org/wiki/PostgreSQL_10_Open_Items

Thanks!

Stephen

On 2017-03-09 13:34:22 -0500, Robert Haas wrote:

On Mon, Jan 30, 2017 at 6:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

Wonder if we there's an argument to be made for implementing this
roughly similarly to split_pathtarget_at_srf - instead of injecting a
ProjectSet node we'd add a FunctionScan node below a Result node.

Yeah, possibly. That would have the advantage of avoiding an ExecProject
step when the SRFs aren't buried, which would certainly be the expected
case.

If you don't want to make ExecInitExpr responsible, then the planner would
have to do something like split_pathtarget_at_srf anyway to decompose the
expressions, no matter which executor representation we use.

Did we do anything about this? Are we going to?

Working on a patch.

- Andres

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

On Fri, Mar 10, 2017 at 11:49:46AM -0800, Andres Freund wrote:

On 2017-03-09 13:34:22 -0500, Robert Haas wrote:

On Mon, Jan 30, 2017 at 6:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

Wonder if we there's an argument to be made for implementing this
roughly similarly to split_pathtarget_at_srf - instead of injecting a
ProjectSet node we'd add a FunctionScan node below a Result node.

Yeah, possibly. That would have the advantage of avoiding an ExecProject
step when the SRFs aren't buried, which would certainly be the expected
case.

If you don't want to make ExecInitExpr responsible, then the planner would
have to do something like split_pathtarget_at_srf anyway to decompose the
expressions, no matter which executor representation we use.

Did we do anything about this? Are we going to?

Working on a patch.

[Action required within three days. This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item. Andres,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.

[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

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

On 2017-04-05 02:47:55 -0400, Noah Misch wrote:

On Fri, Mar 10, 2017 at 11:49:46AM -0800, Andres Freund wrote:

On 2017-03-09 13:34:22 -0500, Robert Haas wrote:

On Mon, Jan 30, 2017 at 6:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

Wonder if we there's an argument to be made for implementing this
roughly similarly to split_pathtarget_at_srf - instead of injecting a
ProjectSet node we'd add a FunctionScan node below a Result node.

Yeah, possibly. That would have the advantage of avoiding an ExecProject
step when the SRFs aren't buried, which would certainly be the expected
case.

If you don't want to make ExecInitExpr responsible, then the planner would
have to do something like split_pathtarget_at_srf anyway to decompose the
expressions, no matter which executor representation we use.

Did we do anything about this? Are we going to?

Working on a patch.

[Action required within three days. This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item. Andres,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.

I've a very preliminary patch. I'd like to only start polishing it up
once the code freeze is over, so I can work on getting some patches in -
note that I myself have no pending patches. Once frozen I'll polish it
up and send that within a few days.

Ok?

- Andres

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

Andres Freund <andres@anarazel.de> writes:

On 2017-04-05 02:47:55 -0400, Noah Misch wrote:

[Action required within three days. This is a generic notification.]

I've a very preliminary patch. I'd like to only start polishing it up
once the code freeze is over, so I can work on getting some patches in -
note that I myself have no pending patches. Once frozen I'll polish it
up and send that within a few days.

FWIW, I'm willing to help out on 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

On Wed, Apr 05, 2017 at 12:16:25AM -0700, Andres Freund wrote:

On 2017-04-05 02:47:55 -0400, Noah Misch wrote:

On Fri, Mar 10, 2017 at 11:49:46AM -0800, Andres Freund wrote:

On 2017-03-09 13:34:22 -0500, Robert Haas wrote:

On Mon, Jan 30, 2017 at 6:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

Wonder if we there's an argument to be made for implementing this
roughly similarly to split_pathtarget_at_srf - instead of injecting a
ProjectSet node we'd add a FunctionScan node below a Result node.

Yeah, possibly. That would have the advantage of avoiding an ExecProject
step when the SRFs aren't buried, which would certainly be the expected
case.

If you don't want to make ExecInitExpr responsible, then the planner would
have to do something like split_pathtarget_at_srf anyway to decompose the
expressions, no matter which executor representation we use.

Did we do anything about this? Are we going to?

Working on a patch.

[Action required within three days. This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item. Andres,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.

I've a very preliminary patch. I'd like to only start polishing it up
once the code freeze is over, so I can work on getting some patches in -
note that I myself have no pending patches. Once frozen I'll polish it
up and send that within a few days.

Ok?

Okay; using my simplistic translator of "a few", I'll look for your next
status update on or before 2017-04-11. As always, feel free to set a
different date.

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

On 2017-01-30 18:54:50 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Wonder if we there's an argument to be made for implementing this
roughly similarly to split_pathtarget_at_srf - instead of injecting a
ProjectSet node we'd add a FunctionScan node below a Result node.

Yeah, possibly. That would have the advantage of avoiding an ExecProject
step when the SRFs aren't buried, which would certainly be the expected
case.

If you don't want to make ExecInitExpr responsible, then the planner would
have to do something like split_pathtarget_at_srf anyway to decompose the
expressions, no matter which executor representation we use.

Working on this I came across a few things:

Splitting things away from the FunctionScan node doesn't work entirely
naturally, due to ORDINALITY. Consider e.g. cases where there's no
function to evaluate anymore, because it got inlined, but we want to
ORDINALITY. We could obviously support FunctionScan nodes without any
associated (or empty) RangeTblFunctions, but that seems awkward.

Secondly, doing non-function stuff gets interesting when volatility is
involved: Consider something like FROM CAST(srf() * volatile_func() AS
whatnot); when, and how often, does volatile_func() get evaluated? If
we put it somewhere around the projection path it'll only get evaluated
if the rows are actually retrieved and will get re-evaluated upon
projection. If we put it somewhere below the tuplestores that
nodeFunctionscan./execSRF.c generate for each RangeTblFunction, it'll
get called evaluated regardless of being retrieved. The latter is
noticeably more complicated, because of SRFM_Materialize SRFs. Our
policy around when it's ok to re-evaluate volatile functions isn't clear
enough to me, to say which one is right and which one is wrong.

For now there's simply another RangeTblFunctions field with a separate
non-FuncExpr expression, that can reference to the SRF output via scan
Vars. That's evaluated in FunctionNext's !simple branch, where we
conveniently have a separate slot already. The separation currently
happens create_functionscan_plan().

Tom, do you have any opinion on the volatility stuff?

- Andres

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

Andres Freund <andres@anarazel.de> writes:

Tom, do you have any opinion on the volatility stuff?

What was the previous behavior for such cases? If it was reasonably
sane, we probably have to preserve it. If it was unpredictable or
completely wacko, maybe we don't.

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