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

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?

> 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?

> > - I continued with the division of Labor that Tom had set up, so we're
> > creating one Srf node for each "nested" set of SRFs. We'd discussed
> > nearby to change that for one node/path for all nested SRFs, partially
> > because of costing. But I don't like the idea that much anymore. The
> > implementation seems cleaner (and probably faster) this way, and I
> > don't think nested targetlist SRFs are something worth optimizing
> > for. Anybody wants to argue differently?
>
> 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 :)

> ? 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.

Greetings,

Andres Freund