On 2017-07-18 15:45:53 -0400, Tom Lane wrote: > Andres Freund <and...@anarazel.de> writes: > > Attached is a trivial patch implementing 1) and a proof-of-concept hack > > for 2). > > The comment you made previously, as well as the proposed commit message > for 0001, suggest that you've forgotten that pre-v10 execQual.c had > several check_stack_depth() calls. Per its header comment,
> * During expression evaluation, we check_stack_depth only in > * ExecMakeFunctionResult (and substitute routines) rather than at > every > * single node. This is a compromise that trades off precision of the > * stack limit setting to gain speed. No, I do remember that fact. But a) unfortunately that's not really that useful because by far not all function calls go through ExecMakeFunctionResult() (e.g. ExecEvalDistinct() and a bunch of other FunctionCallInvoke() containing functions). b) deeply nested executor nodes - and that's what the commit's about to a good degree - aren't necessarily guaranteed to actually evaluate expressions. There's no guarantee there's any expressions (you could just nest joins without conditions), and a bunch of nodes like hashjoins invoke functions themselves. > and there was also a check in the recursive ExecInitExpr routine. Which still is there. > While it doesn't seem to be documented anywhere, I believe that we'd > argued that ExecProcNode and friends didn't need their own stack depth > checks because any nontrivial node would surely do expression evaluation > which would contain a check. Yea, I don't buy that at all. > I agree that adding a check to ExecInitNode() is really necessary, > but I'm not convinced that it's a sufficient substitute for checking > in ExecProcNode(). The two flaws I see in that argument are > > (a) you've provided no hard evidence backing up the argument that node > initialization will take strictly more stack space than node execution; > as far as I can see that's just wishful thinking. I think it's pretty likely to be roughly (within slop) the case in realistic scenarios, but I do feel fairly uncomfortable about the assumption. That's why I really want to do something like the what I'm proposing in the second patch. I just don't think we can realistically add the check to the back branches, given that it's quite measurable performancewise. > (b) there's also an implicit assumption that ExecutorRun is called from > a point not significantly more deeply nested than the corresponding > call to ExecutorStart. This seems also to depend on nothing much better > than wishful thinking. Certainly, many ExecutorRun calls are right next > to ExecutorStart, but several of them aren't; the portal code and > SQL-function code are both scary in this respect. :/ > > The latter obviously isn't ready, but might make clearer what I'm > > thinking about. If we were to go this route we'd have to probably move > > the callback assignment into the ExecInit* routines, and possibly > > replace the switch in ExecInitNode() with another callback, assigned in > > make_*, and implement callbacks for ExecShutdown, MultiExecProcNode etc. > > While I'm uncomfortable with making such a change post-beta2, I'm one > heck of a lot *more* uncomfortable with shipping v10 with no stack depth > checks in the executor mainline. Fleshing this out and putting it in > v10 might be an acceptable compromise. Ok, I'll flesh out the patch till Thursday. But I do think we're going to have to do something about the back branches, too. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers