Re: [HACKERS] segfault in HEAD when too many nested functions call - Mailing list pgsql-hackers

From Tom Lane
Subject Re: [HACKERS] segfault in HEAD when too many nested functions call
Date
Msg-id 19899.1500682674@sss.pgh.pa.us
Whole thread Raw
In response to Re: [HACKERS] segfault in HEAD when too many nested functions call  (Andres Freund <andres@anarazel.de>)
Responses Re: [HACKERS] segfault in HEAD when too many nested functions call
List pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
> On 2017-07-18 16:53:43 -0400, Tom Lane wrote:
>> BTW, I don't see why you really need to mess with anything except
>> ExecProcNode?  Surely the other cases such as MultiExecProcNode are
>> not called often enough to justify changing them away from the
>> switch technology.  Yeah, maybe it would be a bit cleaner if they
>> all looked alike ... but if you're trying to make a patch that's
>> as little invasive as possible for v10, I'd suggest converting just
>> ExecProcNode to this style.

> Yea, I was making that statement when not aiming for v10.  Attached is a
> patch that converts just ExecProcNode.

I read through this patch (didn't test it at all yet).

> I dislike having the miscadmin.h include in executor.h - but I don't
> quite see a better alternative.

I seriously, seriously, seriously dislike that.  You practically might as
well put miscadmin.h into postgres.h.  Instead, what do you think of
requiring the individual ExecProcNode functions to perform
CHECK_FOR_INTERRUPTS?  Since they're already responsible for doing that
if they have any long-running internal loops, this doesn't seem like a
modularity violation.  It is a risk for bugs-of-omission, sure, but so
are a lot of other things that the per-node code has to do.

There might be something to be said for handling the chgParam/rescan tests
similarly.  That would reduce the ExecProcNode macro to a triviality,
which would be a good thing for understandability of the code I think.

Some other thoughts:

* I think the comments need more work.  Am willing to make a pass over
that if you want.

* In most places, if there's an immediate return-if-trivial-case test,
we check stack depth only after that.  There's no point in checking
and then returning; either you already crashed, or you're at peak
stack so far as this code path is concerned.

* Can we redefine the ExecCustomScan function pointer as type
ExecProcNodeCB, eliminating the unsightly cast in nodeCustom.c?

* The various callers of ExecScan() are pretty inconsistently coded.
I don't care that much whether they use castNode() or just forcibly
cast to ScanState*, but let's make them all look the same.

* I believe the usual term for what these function pointers are is
"methods", not "callbacks".  Certainly we'd call them that if we
were working in C++.

> I still think we should backpatch at least the check_stack_depth() calls
> in ExecInitNode(), ExecEndNode().

No big objection, although I'm not sure it's necessary.
        regards, tom lane



pgsql-hackers by date:

Previous
From: Greg Stark
Date:
Subject: Re: [HACKERS] More optimization effort?
Next
From: Peter Geoghegan
Date:
Subject: Re: [HACKERS] [WIP] [B-Tree] Keep indexes sorted by heap physical location