Re: [v9.5] Custom Plan API

> On Wed, Sep 17, 2014 at 7:40 PM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> > At this moment, I revised the above portion of the patches.
> > create_custom_plan() was modified to call "PlanCustomPath" callback
> > next to the initialization of tlist and clauses.
> >
> > It's probably same as what you suggested.
>
> create_custom_plan() is mis-named. It's actually only applicable to the
> custom-scan case, because it's triggered by create_plan_recurse() getting
> a path node with a T_CustomScan pathtype. Now, we could change that;
> although in general create_plan_recurse() dispatches on pathtype, we could
> make CustomPath an exception; the top of that function could say if
> (IsA(best_path, CustomPath)) { /* do custom stuff */ }. But the problem
> with that idea is that, when the custom path is specifically a custom scan,
> rather than a join or some other thing, you want to do all of the same
> processing that's in create_scan_plan().
>
> So I think what should happen is that create_plan_recurse() should handle
> T_CustomScan the same way it handles T_SeqScan, T_IndexScan, et
> al: by calling create_scan_plan(). The switch inside that function can
> then call a function create_customscan_plan() if it sees T_CustomScan. And
> that function will be simpler than the
> create_custom_plan() that you have now, and it will be named correctly,
> too.
>
Fixed, according to what you suggested. It seems to me create_customscan_plan()
became more simplified than before.
Probably, it will minimize the portion of special case handling if CustomScan
with scanrelid==0 replaces built-in join plan in the future version.

> In ExplainNode(), I think sname should be set to "Custom Scan", not "Custom".
> And further down, the custom_name should be printed as "Custom Plan
> Provider" not just "Custom".
>
Fixed. I added an additional regression test to check EXPLAIN output
if not a text format.

> setrefs.c has remaining handling for the scanrelid = 0 case; please remove
> that.
>
Sorry, I removed it, and checked the patch again to ensure here is no similar
portions.

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

On 29 September 2014 09:48, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>> On Wed, Sep 17, 2014 at 7:40 PM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>> > At this moment, I revised the above portion of the patches.
>> > create_custom_plan() was modified to call "PlanCustomPath" callback
>> > next to the initialization of tlist and clauses.
>> >
>> > It's probably same as what you suggested.
>>
>> create_custom_plan() is mis-named. It's actually only applicable to the
>> custom-scan case, because it's triggered by create_plan_recurse() getting
>> a path node with a T_CustomScan pathtype. Now, we could change that;
>> although in general create_plan_recurse() dispatches on pathtype, we could
>> make CustomPath an exception; the top of that function could say if
>> (IsA(best_path, CustomPath)) { /* do custom stuff */ }. But the problem
>> with that idea is that, when the custom path is specifically a custom scan,
>> rather than a join or some other thing, you want to do all of the same
>> processing that's in create_scan_plan().
>>
>> So I think what should happen is that create_plan_recurse() should handle
>> T_CustomScan the same way it handles T_SeqScan, T_IndexScan, et
>> al: by calling create_scan_plan(). The switch inside that function can
>> then call a function create_customscan_plan() if it sees T_CustomScan. And
>> that function will be simpler than the
>> create_custom_plan() that you have now, and it will be named correctly,
>> too.
>>
> Fixed, according to what you suggested. It seems to me create_customscan_plan()
> became more simplified than before.
> Probably, it will minimize the portion of special case handling if CustomScan
> with scanrelid==0 replaces built-in join plan in the future version.
>
>> In ExplainNode(), I think sname should be set to "Custom Scan", not "Custom".
>> And further down, the custom_name should be printed as "Custom Plan
>> Provider" not just "Custom".
>>
> Fixed. I added an additional regression test to check EXPLAIN output
> if not a text format.
>
>> setrefs.c has remaining handling for the scanrelid = 0 case; please remove
>> that.
>>
> Sorry, I removed it, and checked the patch again to ensure here is no similar
> portions.
>
> Thanks for your reviewing.

pgsql-v9.5-custom-scan.part-2.v11.patch

+GetSpecialCustomVar(CustomPlanState *node,
+ Var *varnode,
+ PlanState **child_ps);

This doesn't seem to strictly match the actual function:

+GetSpecialCustomVar(PlanState *ps, Var *varnode, PlanState **child_ps)

--
Thom

2014-09-29 20:26 GMT+09:00 Thom Brown <thom(at)linux(dot)com>:
> On 29 September 2014 09:48, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>>> On Wed, Sep 17, 2014 at 7:40 PM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>>> > At this moment, I revised the above portion of the patches.
>>> > create_custom_plan() was modified to call "PlanCustomPath" callback
>>> > next to the initialization of tlist and clauses.
>>> >
>>> > It's probably same as what you suggested.
>>>
>>> create_custom_plan() is mis-named. It's actually only applicable to the
>>> custom-scan case, because it's triggered by create_plan_recurse() getting
>>> a path node with a T_CustomScan pathtype. Now, we could change that;
>>> although in general create_plan_recurse() dispatches on pathtype, we could
>>> make CustomPath an exception; the top of that function could say if
>>> (IsA(best_path, CustomPath)) { /* do custom stuff */ }. But the problem
>>> with that idea is that, when the custom path is specifically a custom scan,
>>> rather than a join or some other thing, you want to do all of the same
>>> processing that's in create_scan_plan().
>>>
>>> So I think what should happen is that create_plan_recurse() should handle
>>> T_CustomScan the same way it handles T_SeqScan, T_IndexScan, et
>>> al: by calling create_scan_plan(). The switch inside that function can
>>> then call a function create_customscan_plan() if it sees T_CustomScan. And
>>> that function will be simpler than the
>>> create_custom_plan() that you have now, and it will be named correctly,
>>> too.
>>>
>> Fixed, according to what you suggested. It seems to me create_customscan_plan()
>> became more simplified than before.
>> Probably, it will minimize the portion of special case handling if CustomScan
>> with scanrelid==0 replaces built-in join plan in the future version.
>>
>>> In ExplainNode(), I think sname should be set to "Custom Scan", not "Custom".
>>> And further down, the custom_name should be printed as "Custom Plan
>>> Provider" not just "Custom".
>>>
>> Fixed. I added an additional regression test to check EXPLAIN output
>> if not a text format.
>>
>>> setrefs.c has remaining handling for the scanrelid = 0 case; please remove
>>> that.
>>>
>> Sorry, I removed it, and checked the patch again to ensure here is no similar
>> portions.
>>
>> Thanks for your reviewing.
>
> pgsql-v9.5-custom-scan.part-2.v11.patch
>
> +GetSpecialCustomVar(CustomPlanState *node,
> + Var *varnode,
> + PlanState **child_ps);
>
> This doesn't seem to strictly match the actual function:
>
> +GetSpecialCustomVar(PlanState *ps, Var *varnode, PlanState **child_ps)
>
It's more convenient if the first argument is PlanState, because
GetSpecialCustomVar() is called towards all the suspicious special
var-node that might be managed by custom-plan provider.
If we have to ensure its first argument is CustomPlanState on the
caller side, it makes function's invocation more complicated.
Also, the callback portion is called only when PlanState is
CustomPlanState, so it is natural to take CustomPlanState for
argument of the callback interface.
Do we need to match the prototype of wrapper function with callback?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Sep 29, 2014 at 9:04 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> Do we need to match the prototype of wrapper function with callback?

Yes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company