|From:||Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>|
|To:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andres Freund <andres(at)anarazel(dot)de>, "Robert Haas" <robertmhaas(at)gmail(dot)com>|
|Cc:||Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>, Thom Brown <thom(at)linux(dot)com>, "Shigeru Hanada" <shigeru(dot)hanada(at)gmail(dot)com>, "pgsql-hackers(at)postgreSQL(dot)org" <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API)|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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';
> Subject: RE: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
> 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.
> 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.
> 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.
> Enforce authors of custom-scan provider to copy and paste createplan.c.
> I really don't want this option and nobody will be happy.
> NEC Business Creation Division / PG-Strom Project
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)postgreSQL(dot)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 <tgl(at)sss(dot)pgh(dot)pa(dot)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 <kaigai(at)ak(dot)jp(dot)nec(dot)com>
|Next Message||Andres Freund||2015-05-13 01:04:11||Re: a fast bloat measurement tool (was Re: Measuring relation free space)|
|Previous Message||Andres Freund||2015-05-12 23:48:41||Re: Final Patch for GROUPING SETS|