2015-05-09 8:18 GMT+09:00 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2015-05-09 2:46 GMT+09:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>>> I've been trying to code-review this patch, because the documentation
>>>> seemed several bricks shy of a load, and I find myself entirely confused
>>>> by the fdw_ps_tlist and custom_ps_tlist fields.
>>
>>> Main-point of your concern is lack of documentation/comments to introduce
>>> how does the pseudo-scan targetlist works here, isn't it??
>>
>> Well, there's a bunch of omissions and outright errors in the docs and
>> comments, but this is the main issue that I was uncertain how to fix
>> from looking at the patch.
>>
>>>> Also,
>>>> if that is what they're for (ie, to allow the FDW to redefine the scan
>>>> tuple contents) it would likely be better to decouple that feature from
>>>> whether the plan node is for a simple scan or a join.
>>
>>> In this version, we don't intend FDW/CSP to redefine the contents of
>>> scan tuples, even though I want off-loads heavy targetlist calculation
>>> workloads to external computing resources in *the future version*.
>>
>> I do not think it's a good idea to introduce such a field now and then
>> redefine how it works and what it's for in a future version. We should
>> not be moving the FDW APIs around more than we absolutely have to,
>> especially not in ways that wouldn't throw an obvious compile error
>> for un-updated code. Also, the longer we wait to make a change that
>> we know we want, the more pain we inflict on FDW authors (simply because
>> there will be more of them a year from now than there are today).
>>
> Ah, above my sentence don't intend to reuse the existing field for
> different works in the future version. It's just what I want to support
> in the future version.
> Yep, I see. It is not a good idea to redefine the existing field for
> different purpose silently. It's not my plan.
>
>>>> The business about
>>>> resjunk columns in that list also seems a bit half baked, or at least
>>>> underdocumented.
>>
>>> I'll add source code comments to introduce how does it works any when
>>> does it have resjunk=true. It will be a bit too deep to be introduced
>>> in the SGML file.
>>
>> I don't actually see a reason for resjunk marking in that list at all,
>> if what it's for is to define the contents of the scan tuple. I think we
>> should just s/ExecCleanTypeFromTL/ExecTypeFromTL/ in nodeForeignscan and
>> nodeCustom, and get rid of the "sanity check" in create_foreignscan_plan
>> (which is pretty pointless anyway, considering the number of other ways
>> you could screw up that tlist without it being detected).
>>
> http://www.postgresql.org/message-id/9A28C8860F777E439AA12E8AEA7694F8010D7E24@BPXM15GP.gisp.nec.co.jp
>
> Does the introduction in above post make sense?
> The *_ps_tlist is not only used for a basic of scan-tuple descriptor, but
> also used to solve var-node if varno==INDEX_VAR in EXPLAIN command.
> On the other hands, existence of the junk entries (which are referenced in
> external computing resources only) may cause unnecessary projection.
> So, I want to discriminate target-entries for basis of scan-tuple descriptor
> from other ones just for EXPLAIN command.
>
>> I'm also inclined to rename the fields to
>> fdw_scan_tlist/custom_scan_tlist, which would better reflect what they do,
>> and to change the API of make_foreignscan() to add a parameter
>> corresponding to the scan tlist. It's utterly bizarre and error-prone
>> that this patch has added a field that the FDW is supposed to set and
>> not changed make_foreignscan to match.
>>
> OK, I'll do the both of changes. The name of ps_tlist is a shorten of
> "pseudo-scan target-list". So, fdw_scan_tlist/custom_scan_tlist are
> almost intentional.
>
The attached patch renamed *_ps_tlist by *_scan_tlist according to
the suggestion.
Also, put a few detailed source code comments around this alternative
scan_tlist.
Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>