Re: postgres_fdw behaves oddly

On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> wrote:

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