Re: Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API) - Mailing list pgsql-hackers

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
> I briefly checked your updates.
> Even though it is not described in the commit-log, I noticed a
> problematic change.

> This commit reverts create_plan_recurse() as static function.

Yes.  I am not convinced that external callers should be calling that,
and would prefer not to enlarge createplan.c's API footprint without a
demonstration that this is right and useful.  (This is one of many
ways in which this patch is suffering from having gotten committed
without submitted use-cases.)

> It means extension
> cannot have child node, even if it wants to add a custom-join logic.
> Please assume a simple case below:
>   SELECT * FROM t0, t1 WHERE t0.a = t1.x;

> An extension adds a custom join path, then its PlanCustomPath method will be
> called back to create a plan node once it gets chosen by planner.
> The role of PlanCustomPath is to construct a plan-node of itself, and plan-nodes
> of the source relations also.
> If create_plan_recurse() is static, we have no way to initialize
> plan-node for t0
> and t1 scan even if join-logic itself is powerful than built-in ones.

I find this argument quite unconvincing, because even granting that this
is an appropriate way to create child nodes of a CustomScan, there is a
lot of core code besides createplan.c that would not know about those
child nodes either.

As a counterexample, suppose that your cool-new-join-method is capable of
joining three inputs at once.  You could stick two of the children into
lefttree and righttree perhaps, but where are you gonna put the other?

This comes back to the fact that trying to wedge join behavior into scan
node types was a pretty bad idea (as evidenced by the entirely bogus
decision that now scanrelid can be zero, which I rather doubt you've found
all the places that that broke).  Really there should have been a new
CustomJoin node or something like that.  If we'd done that, it would be
possible to design that node type more like Append, with any number of
child nodes.  And we could have set things up so that createplan.c knows
about those child nodes and takes care of the recursion for you; it would
still not be a good idea to expose create_plan_recurse and hope that
callers of that would know how to use it correctly.

I am still more than half tempted to say we should revert this entire
patch series and hope for a better design to be submitted for 9.6.
In the meantime, though, poking random holes in the modularity of core
code is a poor substitute for having designed a well-thought-out API.

A possible compromise that we could perhaps still wedge into 9.5 is to
extend CustomPath with a List of child Paths, and CustomScan with a List
of child Plans, which createplan.c would know to build from the Paths,
and other modules would then also be aware of these children.  I find that
uglier than a separate join node type, but it would be tolerable I guess.
        regards, tom lane