Re: Custom Plan node

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
> The attached patch adds a new plan node type; CustomPlan that enables
> extensions to get control during query execution, via registered callbacks.

TBH, I think this is really an exercise in building useless mechanism.
I don't believe that any actually *interesting* new types of plan node can
be inserted into a query plan without invasive changes to the planner, and
so it's a bit pointless to set up hooks whereby you can avoid touching any
source code in the executor.

> ... Extension will put its local code on the planner_hook
> to reference and manipulate PlannedStmt object.

That is hardly a credible design for doing anything interesting with
custom plans.  It's got exactly the same problem you are complaining about
for the executor, ie you have to replace the whole of the planner if you
try to do things that way.

One other point here is: if you need more than one kind of custom plan
node, how will you tell what's what?  I doubt you can completely eliminate
the need for IsA-style tests, especially in the planner area.  The sample
contrib module here already exposes the failure mode I'm worried about:
it falls down as soon as it sees a plan node type it doesn't know.  If you
could show me how this would work together with some other extension
that's also adding custom plan nodes of its own, then I might think you
had something.

In the same vein, the patch fails to provide credible behavior for
ExecSupportsMarkRestore, ExecMaterializesOutput, ExplainPreScanNode,
search_plan_tree, and probably some other places that need to know
about all possible plan node types.

Even if you'd covered every one of those bases, you've still only got
support for "generic" plan nodes having no particularly unique properties.
As an example of what I'm thinking about here, NestLoop, which might be
considered the most vanilla of all join plan nodes, actually has a lot of
specialized infrastructure in both the planner and the executor to support
its ability to pass outer-relation values into the inner-relation scan.
I think that as soon as you try to do anything of real interest with
custom plan nodes, you'll be finding you need special-purpose additions
that no set of generic hooks could reasonably cover.

In short, I don't understand or agree with this idea that major changes
should be implementable without touching any of the core code in any way.
This is open source --- if you need a modified version then modify it.
I used to build systems that needed hook-style extensibility because the
core code was burned into ROM; but that's not what we're dealing with
today, and I don't really see the argument for sacrificing readability
and performance by putting hooks everywhere, especially in places with
vague, ever-changing API contracts.
        regards, tom lane