Re: postgres_fdw behaves oddly

(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