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

Hi,

On 2017-01-18 08:43:24 -0500, Tom Lane wrote:
> I did a review pass over 0001 and 0002.  I think the attached updated
> version is committable

Cool.

> ... except for one thing.  The more I look at it,
> the more disturbed I am by the behavioral change shown in rangefuncs.out
> --- that's the SRF-in-one-arm-of-CASE issue.  (The changes in tsrf.out
> are fine and as per agreement.)  We touched lightly on that point far
> upthread, but didn't really resolve it.  What's bothering me is that
> we're changing, silently, from a reasonably-intuitive behavior to a
> completely-not-intuitive one.  Since we got a bug report for the previous
> less-than-intuitive behavior for such cases, it's inevitable that we'll
> get bug reports for this.  I think it'd be far better to throw error for
> SRF-inside-a-CASE.  If we don't, we certainly need to document this,
> and I'm not very sure how to explain it clearly.

I'm fine with leaving it as is in the patch, but I'm also fine with
changing things to ERROR.  Personally I don't think it matters much, and
we can whack it back and forth as we want later.  Thus I'm inclined to
commit it without erroring out; since presumably we'll take some time
deciding on what exactly we want to prohibit.


> Anyway, I've not done anything about that in the attached.  What I did do:
> 
> * Merge 0001 and 0002.  I appreciate you having separated that for my
> review, but it doesn't make any sense to commit the parts of 0001 that
> you undid in 0002.

Right. I was suggesting upthread that we'd merge them before committing.


> * Obviously, ExecMakeFunctionResultSet can be greatly simplified now
> that it need not deal with hasSetArg cases.

Yea, I've cleaned it up in my 0003; where it would have started to error
out too (without an explicit check), because there's no set evaluating
function anymore besides ExecMakeFunctionResultSet.


> I saw you'd left that
> for later, which is mostly fine, but I did lobotomize it just enough
> to throw an error if it gets a set result from an argument.  Without
> that, we wouldn't really be testing that the planner splits nested
> SRFs correctly.

Ok, that makes sense.


> * This bit in ExecProjectSRF was no good:
> 
> +         else if (IsA(gstate->arg, FuncExprState) &&
> +                  ((FuncExpr *) gstate->arg->expr)->funcretset)
> 
> because FuncExprState is used for more node types than just FuncExpr;
> in particular this would fail (except perhaps by accident) for a
> set-returning OpExpr.

Argh. That should have been FunExprState->func->fn_retset.  Anyway, your
approach works, too.


> * Update the user documentation (didn't address the CASE issue, though).

Cool.


Greetings,

Andres