Re: [v9.5] Custom Plan API

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