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

Hi,

On 2017-01-16 14:13:18 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > 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?
>
> "Srf" is ugly as can be, and unintelligible. SetResult might be OK.

Named it SetResult - imo looks ok. I think I do prefer the separate
node type over re-using Result. The planner integration looks cleaner
to me due to not needing the srfpp special cases and such.

> > Comments?
>
> Hard to comment on your other points without a patch to look at.

Attached the current version. There's a *lot* of pending cleanup needed
(especially in execQual.c) removing outdated code/comments etc, but this
seems good enough for a first review. I'd want that cleanup done in a
separate patch anyway.

Attached are two patches. The first is just a rebased version (just some
hunk offset changed) of your planner patch, on top of that is my
executor patch. My patch moves some minor detail in yours around, and I
do think they should eventually be merged; but leaving it split for a
round displays the changes more cleanly.

Additional questions:
- do we care about SRFs that don't actually return a set? If so we need
to change the error checking code in ExecEvalFunc/Oper and move it to
the actual invocation.
- the FuncExpr/OpExpr check in ExecMakeFunctionResult is fairly ugly imo
- but I don't quite see a much better solution.

Greetings,

Andres