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

On 2016-09-12 17:36:07 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
> >> Stepping back a little bit ... way back at the start of this thread
> >> you muttered about possibly implementing tSRFs as a special pipeline
> >> node type, a la Result.  That would have the same benefits in terms
> >> of being able to take SRF support out of the main execQual code paths.
> >> I, and I think some other people, felt that the LATERAL approach would
> >> be a cleaner answer --- but now that we're seeing some of the messy
> >> details required to make the LATERAL way work, I'm beginning to have
> >> second thoughts.  I wonder if we should do at least a POC implementation
> >> of the other way to get a better fix on which way is really cleaner.
> 
> > I'm not particularly in love in restarting with a different approach. I
> > think fixing the ROWS FROM expansion is the only really painful bit, and
> > that seems like it's independently beneficial to allow for suppression
> > of expansion there.
> 
> Um, I dunno.  You've added half a thousand lines of not-highly-readable-
> nor-extensively-commented code to the planner; that certainly reaches *my*
> threshold of pain.

Well, I certainly plan (and started to) make that code easier to
understand, and better commented.  It also removes ~1400 LOC of not easy
to understand code... A good chunk of that'd would also be removed with
a Result style approach, but far from all.


> I'm also growing rather concerned that the LATERAL
> approach is going to lock us into some unremovable incompatibilities
> no matter how much we might regret that later (and in view of how quickly
> I got my wrist slapped in <001201d18524$f84c4580$e8e4d080$@pcorp.us>,
> I am afraid there may be more pushback awaiting us than we think).

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.


> 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. Because
handling ExprMultipleResult in all these places is a serious issue
WRT making expression evaluation faster.  If we find a good answer to
that, I'd be more open to pursuing this approach.


> > I actually had started to work on a Result style approach, and I don't
> > think it turned out that nice. But I didn't complete it, so I might just
> > be wrong.
> 
> It's also possible that it's easier now in the post-pathification code
> base than it was before.  After contemplating my navel for awhile, it
> seems like the core of the planner code for a Result-like approach would
> be something very close to make_group_input_target(), plus a thing like
> pull_var_clause() that extracts SRFs rather than Vars.  Those two
> functions, including their lengthy comments, weigh in at ~250 lines.
> Admittedly there'd be some boilerplate on top of that, if we invent a
> new plan node type rather than extending Result, but not all that much.

That's pretty much what I did (or rather started to do), yes.  I had
something that was called from grouping_planner() that added the new
node ontop of the original set of paths, after grouping or after
distinct, depending on where SRFs were referenced.  The biggest benefit
I saw with that is that there's no need to push things into a subquery,
and that the ordering is a lot more explicit.


> If you like, I'll have a go at drafting a patch in that style.  Do you
> happen to still have the executor side of what you did, so I don't have
> to reinvent that?

The executor side is actually what I found harder here. Either we end up
keeping most of ExecQual's handling, or we reinvent a good deal of
separate logic.

Greetings,

Andres Freund