Re: [HACKERS] segfault in HEAD when too many nested functions call

On 2017-07-18 15:45:53 -0400, Tom Lane wrote:
> Andres Freund <andres@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
toactually 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