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