(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