On 2017-07-24 13:27:58 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> >> 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.
Well, that's why I'm pondering an executor_internal.h or something -
there shouldn't be ExecProcNode() callers that don't also need CFI(),
and no executor callers should need ExecProcNode(). executor.h right now
really defines infrastructure to *use* the executor
(Executor{Start,Run,Finish,End,Rewind}), functions internal to the
executor (lots of initialization functions, EPQ, partition logic), some
things inbetween (e.g. expression related stuff), and some things that
really should be separate ExecOpenIndices etc, execReplication.c
functions. But that's not something we can easily clear up just now.
> (Or in short, I'm not really okay with *any* header file including
> miscadmin.h.)
Perhaps that's a sign that we should split it up? It's a weird grab bag
atm. utils/interrupt.h or such would e.g. make sense for for the
*INTERRUPTS, and *CRIT_SECTION macros, as well as ProcessInterrupts()
itself, which imo isn't super well placed in postgres.c
either. Including utils/interrupt.h in a header would be much less
odious in my opinion than including miscadmin.h.
> >> * 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.
Not that I can see - I've build & tested citus which uses custom scans
these days with and without patch without trouble. Nor do I see any
change in the current patch that'd be troublesome - after all the
API of ExecProcNode() stays the same.
- Andres