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