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

Hi,

On 2017-01-30 16:55:56 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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