Re: [v9.5] Custom Plan API

> I personally don't see how this patch is 'ready for committer'. I realize
> that that state is sometimes used to denote that review needs to be
> "escalated", but it still seemspremature.
>
> Unless I miss something there hasn't been any API level review of this?
> Also, aren't there several open items?
>
Even though some interface specifications are revised according to the
comment from Tom on the last development cycle, the current set of
interfaces are not reviewed by committers. I really want this.

Here are two open items that we want to wait for committers comments.

* Whether set_cheapest() is called for all relkind?

This pactch moved set_cheapest() to the end of set_rel_pathlist(),
to consolidate entrypoint of custom-plan-provider handler function.
It also implies CPP can provider alternative paths towards non-regular
relations (like sub-queries, functions, ...).
Hanada-san wonder whether we really have a case to run alternative
sub-query code. Even though I don't have usecases for alternative
sub-query execution logic, but we also don't have a reason why not
to restrict it.

* How argument of add_path handler shall be derivered?

The handler function (that adds custom-path to the required relation
scan if it can provide) is declared with an argument with INTERNAL
data type. Extension needs to have type-cast on the supplied pointer
to customScanArg data-type (or potentially customHashJoinArg and
so on...) according to the custom plan class.
I think it is well extendable design than strict argument definitions,
but Hanada-san wonder whether it is the best design.

> Perhaps there needs to be a stage between 'needs review' and 'ready for
> committer'?
>
It needs clarification of 'ready for committer'. I think interface
specification is a kind of task to be discussed with committers
because preference/viewpoint of rr-reviewer are not always same
opinion with them.

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

> -----Original Message-----
> From: Andres Freund [mailto:andres(at)2ndquadrant(dot)com]
> Sent: Friday, July 18, 2014 3:12 AM
> To: Shigeru Hanada
> Cc: Kaigai Kouhei(海外 浩平); Kohei KaiGai; Simon Riggs; Tom Lane; Stephen
> Frost; Robert Haas; PgHacker; Jim Mlodgenski; Peter Eisentraut
> Subject: Re: [HACKERS] [v9.5] Custom Plan API
>
> On 2014-07-16 10:43:08 +0900, Shigeru Hanada wrote:
> > Kaigai-san,
> >
> > 2014-07-15 21:37 GMT+09:00 Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> > > Sorry, expected result of sanity-check test was not updated on
> > > renaming to pg_custom_plan_provider.
> > > The attached patch fixed up this point.
> >
> > I confirmed that all regression tests passed, so I marked the patch as
> > "Ready for committer".
>
> I personally don't see how this patch is 'ready for committer'. I realize
> that that state is sometimes used to denote that review needs to be
> "escalated", but it still seemspremature.
>
> Unless I miss something there hasn't been any API level review of this?
> Also, aren't there several open items?
>
> Perhaps there needs to be a stage between 'needs review' and 'ready for
> committer'?
>
> Greetings,
>
> Andres Freund
>
> --
> Andres Freund http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services