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. Thanks, -- KaiGai Kohei <kai...@kaigai.gr.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
