Re: 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 that does the stack
and potentially other checks. That also makes ExecProcNode() small
enough to be inlined, which ends up being good for jump target
prediction. I've done something similar for v11 for expression
evaluation, getting rid 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()'s allowing us
to put a check_stack_depth() into the former. That seems like it's
required anyway, since in many cases that's going to trigger
stack-depth exhaustion first anyway (unless we hit it in parse
analysis, which also seems quite common).

Greetings,

Andres Freund