> Kohei KaiGai <kai...@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.) > 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 <kai...@ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
