Re: Custom/Foreign-Join-APIs (Re: [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@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 <tgl@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 <kaigai@ak.jp.nec.com>