On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp > wrote:
> (2014/11/19 14:58), Ashutosh Bapat wrote: > >> On Wed, Nov 19, 2014 at 11:18 AM, Etsuro Fujita >> <fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@lab.ntt.co.jp>> wrote: >> (2014/11/18 18:27), Ashutosh Bapat wrote: >> On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita >> <fujita.ets...@lab.ntt.co.jp >> (2014/11/17 19:36), Ashutosh Bapat wrote: >> > > Here are my comments about the patch >> fscan_reltargetlist.patch >> > > 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. >> > > create_foreignscan_plan() is called from create_scan_plan(), which >> passes the tlist. The later uses function use_physical_tlist() >> to decide >> which targetlist should be used (physical or actual). As per >> code below >> in this function >> >> 485 /* >> 486 * We can do this for real relation scans, subquery >> scans, >> function scans, >> 487 * values scans, and CTE scans (but not for, eg, >> joins). >> 488 */ >> 489 if (rel->rtekind != RTE_RELATION && >> 490 rel->rtekind != RTE_SUBQUERY && >> 491 rel->rtekind != RTE_FUNCTION && >> 492 rel->rtekind != RTE_VALUES && >> 493 rel->rtekind != RTE_CTE) >> 494 return false; >> 495 >> 496 /* >> 497 * Can't do it with inheritance cases either (mainly >> because >> Append >> 498 * doesn't project). >> 499 */ >> 500 if (rel->reloptkind != RELOPT_BASEREL) >> 501 return false; >> >> For foreign tables as well as the tables under inheritance >> hierarchy it >> uses the actual targetlist, which contains only the required >> columns IOW >> rel->reltargetlist (see build_path_tlist()) with nested loop >> parameters >> substituted. So, it never included unnecessary columns in the >> targetlist. >> > > Maybe I'm missing something, but I think you are overlooking the >> case for foreign tables that are *not* under an inheritance >> hierarchy. (Note that the rtekind for foreign tables is >> RTE_RELATION.) >> > > Ah! you are right. I confused between rtekind and relkind. Sorry for >> that. May be we should modify use_physical_tlist() to return false in >> case of RELKIND_FOREIGN_TABLE, so that we can use tlist in >> create_foreignscan_plan(). I do not see any create_*_plan() function >> using reltargetlist directly. >> > > Yeah, I think we can do that, but I'm not sure that we should use tlist in > create_foreignscan_plan(), not rel->reltargetlist. How about leaving this > for committers to decide. > > I am fine with that. May be you want to add an XXX comment there to bring it to the committer's notice. > > Thanks, > > Best regards, > Etsuro Fujita > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
