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

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