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

Hi,


On 2016-09-12 13:26:20 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2016-09-12 12:10:01 -0400, Tom Lane wrote:
> >> 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.
> 
> I just remembered that we allow zero-column composite types, which
> makes this proposal formally ambiguous.

Well, we errored out in the grammar for AS () so far... We might want to
fix that independently.


> 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.  I'm working on this to actually be finally able to
get some stuff from the "faster executor" thread in a committable
shape,...  The other stuff like making SELECT * FROM func; not
materialize also seems independently useful; it's something people have
complained about repeatedly over the years.


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.


> Also, one of the points that's come up repeatedly in these discussions
> is the way that the parser's implementation of *-expansion sucks for
> composite-returning functions.  That is, if you write
>     SELECT (foo(...)).* FROM ...
> you get
>     SELECT (foo(...)).col1, (foo(...)).col2, ... FROM ...
> so that the function is executed N times not once.  We had discussed
> fixing that for setof-composite-returning functions by folding multiple
> identical SRF calls into a single LATERAL entry, but that doesn't
> directly fix the problem for non-SRF composite functions.  Also the
> whole idea of having the planner undo the parser's damage in this way
> is kinda grotty, not least because we can't safely combine multiple
> calls of volatile functions, so it only works for not-volatile ones.


> That line of thought leads to the idea that if we could have the *parser*
> do the transformation to LATERAL form, we could avoid breaking a
> composite-returning function call into multiple copies in the first place.
> I had said that I didn't think we wanted this transformation done in the
> parser, but maybe this is a sufficient reason to do so.

I still don't like doing all this is in the parser. It'd just trigger
complaints of users that we're changing their query structure, and we'd
have to solve a good bit of the same problems we have to solve here.

If we really want to reduce the expansion cost - and to me that's a
largely independent issue from this - it seems better to have the parser
emit some structure that's easily recognized at plan time, rather than
have the praser do all the work.


Andres