Andres Freund <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers