Hello, I tried to make patches for the three approaches. Please don't think the option-3 serious proposition, however, it is the only solution at this moment unfortunately.
In my understanding, we don't guarantee interface compatibility across major version up, including the definitions of non-static functions. It is role of extension's author to follow up the new major version (and raise a problem report during development cycle if feature update makes problems without alternatives). In fact, people usually submit patches and a part of them changes definition of non-static functions, however, nobody can guarantee no extension uses this function thus don't break compatibility. It is a collateral evidence we don't think non-static functions are not stable interface for extensions, and it shall not be a reason why to prohibit functions in spite of its necessity. On the other hands, I understand it is not only issues around createplan.c, but also a (philosophical) issue around criteria and human's decision which functions should be static or non-static. So, it usually takes time to get overall consensus. If we keep the create_plan_recurse() static, the option-2 is a solution to balance both of opinions. Anyway, I really dislike the option-3, want to have a solution. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei <kai...@ak.jp.nec.com> > -----Original Message----- > From: Kaigai Kouhei(海外 浩平) > Sent: Tuesday, May 12, 2015 10:24 AM > To: 'Andres Freund'; 'Robert Haas' > Cc: 'Tom Lane'; 'Kohei KaiGai'; 'Thom Brown'; 'Shigeru Hanada'; > 'pgsql-hackers@postgreSQL.org' > Subject: RE: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API) > > Hi, > > Let me back to the original concern and show possible options > we can take here. At least, the latest master is not usable to > implement custom-join logic without either of these options. > > option-1) > Revert create_plan_recurse() to non-static function for extensions. > It is the simplest solution, however, it is still gray zone which > functions shall be public and whether we deal with the non-static > functions as a stable API or not. > IMO, we shouldn't treat non-static functions as stable APIs, even > if it can be called by extensions not only codes in another source > file. In fact, we usually changes definition of non-static functions > even though we know extensions uses. It is role of extension to > follow up the feature across major version. > > > option-2) > Tom's suggestion. Add a new list field of Path nodes on CustomPath, > then create_customscan_plan() will call static create_plan_recurse() > function to construct child plan nodes. > Probably, the attached patch will be an image of this enhancement, > but not tested yet, of course. Once we adopt this approach, I'll > adjust my PG-Strom code towards the new interface within 2 weeks > and report problems if any. > > > option-3) > Enforce authors of custom-scan provider to copy and paste createplan.c. > I really don't want this option and nobody will be happy. > > Thanks, > -- > NEC Business Creation Division / PG-Strom Project > KaiGai Kohei <kai...@ak.jp.nec.com> > > > > -----Original Message----- > > From: Kaigai Kouhei(海外 浩平) > > Sent: Monday, May 11, 2015 12:48 PM > > To: 'Andres Freund'; Robert Haas > > Cc: Tom Lane; Kohei KaiGai; Thom Brown; Shigeru Hanada; > > pgsql-hackers@postgreSQL.org > > Subject: RE: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API) > > > > > On 2015-05-10 21:26:26 -0400, Robert Haas wrote: > > > > On Sun, May 10, 2015 at 8:41 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > > > 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.) > > > > > > Wasn't there a submitted use case? IIRC Kaigai had referenced some > > > pg-strom (?) code using it? > > > > > > I'm failing to see how create_plan_recurse() being exposed externally is > > > related to "having gotten committed without submitted use-cases". Even > > > if submitted, presumably as simple as possible code, doesn't use it, > > > that's not a proof that less simple code does not need it. > > > > > Yes, PG-Strom code uses create_plan_recurse() to construct child plan > > node of the GPU accelerated custom-join logic, once it got chosen. > > Here is nothing special. It calls create_plan_recurse() as built-in > > join path doing on the underlying inner/outer paths. > > It is not difficult to submit as a working example, however, its total > > code size (excludes GPU code) is 25KL at this moment. > > > > I'm not certain whether it is a simple example. > > > > > > Your unwillingness to make functions global or to stick PGDLLIMPORT > > > > markings on variables that people want access to is hugely > > > > handicapping extension authors. Many people have complained about > > > > that on multiple occasions. Frankly, I find it obstructionist and > > > > petty. > > > > > > While I don't find the tone of the characterization super helpful, I do > > > tend to agree that we're *far* too conservative on that end. I've now > > > seen a significant number of extension that copied large swathes of code > > > just to cope with individual functions not being available. And even > > > cases where that lead to minor forks with such details changed. > > > > > I may have to join the members? > > > > > I know that I'm "fearful" of asking for functions being made > > > public. Because it'll invariably get into a discussion of merits that's > > > completely out of proportion with the size of the change. And if I, who > > > has been on the list for a while now, am "afraid" in that way, you can > > > be sure that others won't even dare to ask, lest argue their way > > > through. > > > > > > I think the problem is that during development the default often is to > > > create function as static if they're used only in one file. Which is > > > fine. But it really doesn't work if it's a larger battle to change > > > single incidences. Besides the pain of having to wait for the next > > > major release... > > > > > Thanks, > > -- > > NEC Business Creation Division / PG-Strom Project > > KaiGai Kohei <kai...@ak.jp.nec.com>
custom-join-problem-option-2.v1.patch
Description: custom-join-problem-option-2.v1.patch
custom-join-problem-option-1.v1.patch
Description: custom-join-problem-option-1.v1.patch
custom-join-problem-option-3.v1.patch
Description: custom-join-problem-option-3.v1.patch
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
