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.
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
