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

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 <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';
> 'pgsql-hackers(at)postgreSQL(dot)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 <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>