Re: Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API)

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.

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