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

Kouhei Kaigai <kaigai@ak.jp.nec.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).

>> 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).

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.

>> I do not think that this should have gotten committed without an attendant
>> proof-of-concept patch to postgres_fdw, so that the logic could be tested.

> Hanada-san is now working according to the comments from Robert.

That's nice, but 9.5 feature freeze is only a week away.  I don't have a
lot of confidence that this stuff is actually in a state where we won't
regret shipping it in 9.5.
        regards, tom lane