Re: postgres_fdw behaves oddly - Mailing list pgsql-hackers

(2014/11/19 14:58), Ashutosh Bapat wrote:
> On Wed, Nov 19, 2014 at 11:18 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@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.etsuro@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