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

On 2017-01-15 19:29:52 -0800, Andres Freund wrote:
> On 2016-10-31 09:06:39 -0700, Andres Freund wrote:
> > On 2016-09-14 19:28:25 -0400, Tom Lane wrote:
> > > Andres Freund <andres(at)anarazel(dot)de> writes:
> > > > On 2016-09-12 19:35:22 -0400, Tom Lane wrote:
> > > Here's a draft patch that is just meant to investigate what the planner
> > > changes might look like if we do it in the pipelined-result way.
> > > Accordingly, I didn't touch the executor, but just had it emit regular
> > > Result nodes for SRF-execution steps. However, the SRFs are all
> > > guaranteed to appear at top level of their respective tlists, so that
> > > those Results could be replaced with something that works like
> > > nodeFunctionscan.
> >
> > > So I think we should continue investigating this way of doing things.
> > > I'll try to take a look at the executor end of it tomorrow. However
> > > I'm leaving Friday for a week's vacation, and may not have anything to
> > > show before that.
> >
> > Are you planning to work on the execution side of things? I otherwise
> > can take a stab...
>
> Doing so now.

That worked quite well. So we have a few questions, before I clean this
up:

- For now the node is named 'Srf' both internally and in explain - not
sure if we want to make that something longer/easier to understand for
others? Proposals? TargetFunctionScan? SetResult?

- We could alternatively add all this into the Result node - it's not
actually a lot of new code, and most of that is boilerplate stuff
about adding a new node. I'm ok with both.

- I continued with the division of Labor that Tom had set up, so we're
creating one Srf node for each "nested" set of SRFs. We'd discussed
nearby to change that for one node/path for all nested SRFs, partially
because of costing. But I don't like the idea that much anymore. The
implementation seems cleaner (and probably faster) this way, and I
don't think nested targetlist SRFs are something worth optimizing
for. Anybody wants to argue differently?

- I chose to error out if a retset function appears in ExecEvalFunc/Oper
and make both unconditionally set evalfunc to
ExecMakeFunctionResultNoSets. ExecMakeFunctionResult() is now
externally visible. That seems like the least noisy way to change
things over, but I'm open for different proposals.

Comments?

Regards,

Andres