Hi Fujita-san, Here are my comments about the patch fscan_reltargetlist.patch Sanity -------- Patch applies and compiles cleanly. Regressions in test/regress folder and postgres_fdw and file_fdw are clean.
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. 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. Otherwise, the patch looks good. On Wed, Nov 12, 2014 at 12:55 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp > wrote: > (2014/11/11 18:45), Etsuro Fujita wrote: > >> (2014/11/10 20:05), Ashutosh Bapat wrote: >> >>> Since two separate issues 1. using reltargetlist instead of attr_needed >>> and 2. system columns usage in FDW are being tackled here, we should >>> separate the patch into two one for each of the issues. >>> >> >> Agreed. Will split the patch into two. >> > > Here are splitted patches: > > fscan-reltargetlist.patch - patch for #1 > postgres_fdw-syscol.patch - patch for #2 > > > Thanks, > > Best regards, > Etsuro Fujita > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
