(2014/11/17 19:36), Ashutosh Bapat wrote:
Here are my comments about the patch fscan_reltargetlist.patch
Thanks for the review!
1. This isn't your change, but we might be able to get rid of assignment
2071 /* Are any system columns requested from rel? */
2072 scan_plan->fsSystemCol = false;
since make_foreignscan() already does that. But I will leave upto you to
remove this assignment or not.
As you pointed out, the assignment is redundant, but I think that that'd
improve the clarity and readability. So, I'd vote for leaving that as is.
2. Instead of using rel->reltargetlist, we should use the tlist passed
by caller. This is the tlist expected from the Plan node. For foreign
scans it will be same as rel->reltargetlist. Using tlist would make the
function consistent with create_*scan_plan functions.
I disagree with that for the reasons mentioned below:
* For a foreign scan, tlist is *not* necessarily the same as
rel->reltargetlist (ie, there is a case where tlist contains all user
attributes while rel->reltargetlist contains only attributes actually
needed by the query). In such a case it'd be inefficient to use tlist
rather than rel->reltargetlist.
* I think that it'd improve the readability to match the code with other
places that execute similar processing, such as check_index_only() and
remove_unused_subquery_outputs().
Thanks,
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
