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

Hi,

On 2017-07-15 11:22:37 -0400, Tom Lane wrote:
> The thread drifted off without any resolution, but clearly we need to
> do something before 10.0 final.

We need to do something, I'm less convinced that it's really v10
specific :/


> "SELECT so()" has a nontrivial targetlist so we end up running
> ExecBuildProjectionInfo on that, meaning that a fresh expression
> compilation happens at every nesting depth, and there are
> check_stack_depth calls in expression compilation.  Surely that's
> something we'd try to improve someday.  Or it could accidentally get
> broken by unrelated changes in the way plpgsql sets up queries to be
> executed.

Independent of $subject: What are you thinking of here? You want to
avoid the ExecBuildProjectionInfo() in more cases - I'm unconvinced
that's actually helpful. In my WIP JIT compilation patch the
ExecBuildProjectionInfo() ends up being a good bit faster than paths
avoiding it.


> I still think that we really need to add a check in ExecProcNode().
> Even if there's an argument to be made that every recursion would
> somewhere go through ExecMakeTableFunctionResult, very large/complex
> queries could result in substantial stack getting chewed up before
> we get to that --- and we don't have an infinite amount of stack slop.

I'm less convinced of that, due to the overhead argument.  I think
there's a couple ways around that however:

1) We could move ExecProcNode() to be callback based. The first time a  node is executed a "wrapper" function is called
thatdoes the stack  and potentially other checks. That also makes ExecProcNode() small  enough to be inlined, which
endsup being good for jump target  prediction.   I've done something similar for v11 for expression  evaluation,
gettingrid of EEOP_*_FIRST duplication etc, and it seems  to work well.  The big disadvantage to that is that it's a
bit invasive for v10, and very likely too invasive to backpatch.
 
2) I think there's some fair argument to be made that ExecInitNode()'s  stack-space needs are similar enough to
ExecProcNode()'sallowing us  to put a check_stack_depth() into the former.  That seems like it's  required anyway,
sincein many cases that's going to trigger  stack-depth exhaustion first anyway (unless we hit it in parse  analysis,
whichalso seems quite common).
 

Greetings,

Andres Freund