2015-05-09 8:18 GMT+09:00 Kohei KaiGai <kai...@kaigai.gr.jp>: > 2015-05-09 2:46 GMT+09:00 Tom Lane <t...@sss.pgh.pa.us>: >> Kouhei Kaigai <kai...@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). >> > 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/9a28c8860f777e439aa12e8aea7694f8010d7...@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 <kai...@kaigai.gr.jp>
custom-join-rename-ps_tlist.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
