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

> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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.)
>
Hmm. I got it is intentional change.

> > 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.
>
At this moment, my custom-join logic add a dummy node to have two child
nodes when it tries to join more than 3 relations.
Yep, if CustomPath node (ForeignPath also?) can have a list of child-path
nodes then core backend handles its initialization job, it will be more
comfortable for extensions.
I prefer this idea, rather than agree.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>