Re: Implement targetlist SRFs using ROWS FROM() (was Changed 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