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

Hi,

On 2016-09-12 18:35:03 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I don't think it'd be all that hard to add something like the current
> > LCM behaviour into nodeFunctionscan.c if we really wanted. But I think
> > it'll be better to just say no here.
> 
> "Just say no" soon translates to memes about "disasters like the removal
> of implicit casting" (which in fact is not what 8.3 did, but I've grown
> pretty damn tired of the amount of bitching that that cleanup did and
> still does provoke).  In any case, it feels like the LATERAL approach is
> locking us into more and subtler incompatibilities than just that one.

I can't see those being equivalent impact-wise. Note that the person
bitching most loudly about the "implicit casting" thing (Merlin Moncure)
is voting to remove the LCM behaviour ;)

I think we'll continue to get more bitching about the confusing LCM
behaviour than complaints the backward compat break would generate.


> >> If we go with a Result-like tSRF evaluation node, then whether we change
> >> semantics or not becomes mostly a matter of what that node does.  It could
> >> become basically a wrapper around the existing ExecTargetList() logic if
> >> we needed to provide backwards-compatible behavior.
> 
> > As you previously objected: If we keep ExecTargetList() style logic, we
> > need to keep most of execQual.c's handling of ExprMultipleResult et al,
> > and that's going to prevent the stuff I want to work on.
> 
> You're inventing objections.

Heh, it's actually your own objection ;)

http://archives.postgresql.org/message-id/32673.1464023429%40sss.pgh.pa.us


> It won't require that any more than the
> LATERAL approach does; it's basically the same code as whatever
> nodeFunctionscan is going to do, but packaged as a pipeline eval node
> rather than a base scan node.  Or to be clearer: what I'm suggesting it
> would contain is ExecTargetList's logic about restarting individual SRFs.
> That wouldn't propagate into execQual because we would only allow SRFs at
> the top level of the node's tlist, just like nodeFunctionscan does.
> ExecMakeTableFunctionResult doesn't require the generic execQual code
> to support SRFs today, and it still wouldn't.

(it accidentally does (see your cast example), but that that's besides
your point)

That might work.  It gets somewhat nasty though because you also need to
handle, SRF arguments to SRFs. And those again can contain nearly
arbitrary expressions inbetween. With the ROWS FROM approach that can be
fairly easily handled via LATERAL.  I guess what we could do here is to
use one pipeline node to evaluate all the argument SRFs, and then
another for the outer expression. Where the outer node would evaluate
the SRF arguments using normal expression evaluation, with the inner SRF
output replaced by a var.

I wonder how much duplication we'd end up between nodeFunctionscan.c and
nodeSRF (or whatever). We'd need the latter node to support ValuePerCall
in an non-materializing fashion as well.  Could we combine them somehow?


> In larger terms: the whole point here is to fish SRF calls up to the
> top level of the tlist of whatever node is executing them, where they
> can be special-cased by that node.  Their SRF-free argument
> expressions would be evaluated by generic execQual.  AFAICS this goes
> through in the same way from the executor's viewpoint whether we use
> LATERAL as the query restructuring method or a SRF-capable variant of
> Result.  But it's now looking to me like the latter would be a lot
> simpler from the point of view of planner complexity, and in
> particular from the point of view of proving correctness (equivalence
> of the query transformation).

It's nicer not to introduce subqueries from a two angles from my pov:
1) EXPLAINs will look more like the original query
2) The evaluation order of the non-srf part of the query, and the query  itself, will be easier. That's by the thing
I'mleast happy with the  LATERAL approach.
 

I don't think the code for adding these intermediate SRF evaluating
nodes will be noticeably simpler than what's in my prototype. We'll
still have to do the whole conversion recursively, we'll still need
complexity of figuring out whether to put those SRFs evaluations
before/after group by, order by, distinct on and window functions.

Greetings,

Andres Freund