Andres Freund <and...@anarazel.de> writes: > 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. Well, it still ends up that the callers of ExecProcNode need to include miscadmin.h, whereas if we move it into the per-node functions, then the per-node files need to include miscadmin.h. I think the latter is better because those files may need to have other CHECK_FOR_INTERRUPTS calls anyway. It's less clear from a modularity standpoint that executor callers should need miscadmin.h. (Or in short, I'm not really okay with *any* header file including miscadmin.h.) >> * 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. Agreed, I'll wait till you produce another version. >> * 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. Is this patch really not changing anything else that a custom-scan extension would touch? If not, I'm okay with postponing this bit of cleanup to v11. 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