Re: [v9.5] Custom Plan API

> On Mon, Sep 15, 2014 at 8:38 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
> >> > The only reason why I put separate hooks here is,
> >> > create_custom_scan() needs to know exact size of the CustomScan
> >> > node (including private fields), however, it is helpful for
> >> > extensions to kick its callback to initialize the node next to the
> common initialization stuff.
> >>
> >> Why does it need to know that?  I don't see that it's doing anything
> >> that requires knowing the size of that node, and if it is, I think it
> >> shouldn't be.  That should get delegated to the callback provided by
> >> the custom plan provider.
> >>
> > Sorry, my explanation might be confusable. The create_custom_scan()
> > does not need to know the exact size of the CustomScan (or its
> > inheritance) because of the two separated hooks; one is node
> > allocation time, the other is the tail of the series of initialization.
> > If we have only one hook here, we need to have a mechanism to informs
> > create_custom_scan() an exact size of the CustomScan node; including
> > private fields managed by the provider, instead of the first hook on
> > node allocation time. In this case, node allocation shall be processed
> > by create_custom_scan() and it has to know exact size of the node to be
> allocated.
> >
> > How do I implement the feature here? Is the combination of static node
> > size and callback on the tail more simple than the existing design
> > that takes two individual hooks on create_custom_scan()?
> 
> I still don't get it.  Right now, the logic in create_custom_scan(), which
> I think should really be create_custom_plan() or
> create_plan_from_custom_path(), basically looks like this:
> 
> 1. call hook function CreateCustomPlan
> 2. compute values for tlist and clauses
> 3. pass those values to hook function InitCustomScan() 4. call
> copy_path_costsize
> 
> What I think we should do is:
> 
> 1. compute values for tlist and clauses
> 2. pass those values to hook function PlanCustomPath(), which will return
> a Plan 3. call copy_path_costsize
> 
> create_custom_scan() does not need to allocate the node.  You don't need
> the node to be allocated before computing tlist and clauses.
> 
Thanks, I could get the point.
I'll revise the patch according to the suggestion above.


It seems to me, we can also apply similar manner on ExecInitCustomScan().
The current implementation doing is:
1. call CreateCustomScanState() to allocate a CustomScanState node
2. common initialization of the fields on CustomScanState, but not private  fields.
3. call BeginCustomScan() to initialize remaining stuffs and begin execution.

If BeginCustomScan() is re-defined to accept values for common initialization
portions and to return a CustomScanState node, we may be able to eliminate
the CreateCustomScanState() hook.

Unlike create_custom_scan() case, it takes more number of values for common
initialization portions; expression tree of tlist and quals, scan and result
tuple-slot, projection info and relation handler. It may mess up the interface
specification.
In addition, BeginCustomScan() has to belong to CustomScanMethods, not
CustomexecMethods. I'm uncertain whether it is straightforward location.
(a whisper: It may not need to be separate tables. CustomScan always
populates CustomScanState, unlike relationship between CustomPath and
CustomScan.)

How about your opinion to apply the above manner on ExecInitCustomScan()
also?

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei <kaigai@ak.jp.nec.com>