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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers