Re: segfault in HEAD when too many nested functions call

On 2017-07-18 15:45:53 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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