(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.
Thanks,
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
