I did a review pass over 0001 and 0002. I think the attached updated
version is committable ... 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.
Upthread we had put COALESCE in the same bucket, but I think that's less
of a problem, because in typical usages the SRF would be in the first
argument and so users wouldn't be expecting conditional evaluation.
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.
* Rename the node types as per yesterday's discussion.
* Require Project to always have an input plan node.
* Obviously, ExecMakeFunctionResultSet can be greatly simplified now
that it need not deal with hasSetArg cases. 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.
* 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. I chose to fix it by adding a funcReturnsSet
field to FuncExprState and insisting that ExecInitExpr fill that in
immediately, which it can do easily.
* Minor style and comment improvements; fix a couple small oversights
such as missing outfuncs.c support.
* Update the user documentation (didn't address the CASE issue, though).
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers