Hi, On 2017-07-21 20:17:54 -0400, Tom Lane wrote: > > 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.
That'd work. Another alternative would be to move the inline definition of ExecProcNode() (and probably a bunch of other related functions) into a more internals oriented header. It seems likely that we're going to add more inline functions to the executor, and that'd reduce the coupling of external and internal users a bit. > * I think the comments need more work. Am willing to make a pass over > that if you want. That'd be good, but let's wait till we have something more final. > * 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. I went back/forth a bit on that one. The calling code might call other functions that go deeper on the stack, which won't have the checks. Fine with moving, just wanted to explain why I got there. > * Can we redefine the ExecCustomScan function pointer as type > ExecProcNodeCB, eliminating the unsightly cast in nodeCustom.c? That'd change an "extension API", which is why I skipped it at this point of the release cycle. It's not like we didn't have this type of cast all over before. Ok, with changing it, but that's where I came down. > * 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 tried changed the minimum, perfectly fine to move to castNode in a wholesale manner. Btw, I really want to get rid of ExecScan(), at least as an external function. Does a lot of unnecessary things in a lot of cases, and makes branch prediction a lot worse. Not v10 stuff tho. > * 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++. K. - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers