Re: [HACKERS] Implement targetlist SRFs using ROWS FROM() (wasChanged SRF in targetlist handling)

Andres Freund wrote:
> On 2017-01-16 12:17:46 -0300, Alvaro Herrera wrote:
> > Andres Freund wrote:
> > 
> > > 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.
> > 
> > Hmm.  I wonder if your stuff could be used as support code for
> > XMLTABLE[1].
> 
> I don't immediately see what functionality overlaps, could you expand on
> that?

Well, I haven't read any previous patches in this area, but the xmltable
patch adds a new way of handling set-returning expressions, so it
appears vaguely related.  These aren't properly functions in the current
sense of the word, though.  There is some parallel to what
ExecMakeFunctionResult does, which I suppose is related.

> > Currently it has a bit of additional code of its own,
> > though admittedly it's very little code executor-side.  Would you mind
> > sharing a patch, or more details on how it works?
> 
> Can do both; cleaning up the patch now. What we're talking about here is
> a way to implement targetlist SRF that is based on:
> 
> 1) a patch by Tom that creates additional Result (or now Srf) executor
>    nodes containing SRF evaluation. This guarantees that only Result/Srf
>    nodes have to deal with targetlist SRF evaluation.
> 
> 2) new code to evaluate SRFs in the new Result/Srf node, that doesn't
>    rely on ExecEvalExpr et al. to have a IsDone argument. Instead
>    there's special code to handle that in the new node. That's possible
>    because it's now guaranted that all SRFs are "toplevel" in the
>    relevant targetlist(s).
> 
> 3) Removal of all nearly tSRF related code execQual.c and other
>    executor/ files, including the node->ps.ps_TupFromTlist checks
>    everywhere.
> 
> makes sense?

Hmm, okay.  (The ps_TupFromTlist thing has long seemed an ugly
construction.)  I think the current term for this kind of thing is
TableFunction -- are you really naming this "Srf" literally?  It seems
strange, but maybe it's just me.

> > Nested targetlist SRFs make my head spin.  I suppose they may have some
> > use, but where would you really want this:
> 
> I think there's some cases where it can be useful. Targetlist SRFs as a
> whole really are much more about backward compatibility than anything :)

Sure.

> > ?  If supporting the former makes it harder to support/optimize more
> > reasonable cases, it seems fair game to leave them behind.
> 
> I don't want to desupport them, just don't want to restructure (one node
> doing several levels of SRFs, instead of one per level) just to make it
> easier to give good estimates.

No objections.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services