Re: postgres_fdw behaves oddly

On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> (2014/11/17 19:36), Ashutosh Bapat wrote:
>
>> Here are my comments about the patch fscan_reltargetlist.patch
>>
>
> Thanks for the review!
>
> 1. This isn't your change, but we might be able to get rid of assignment
>> 2071 /* Are any system columns requested from rel? */
>> 2072 scan_plan->fsSystemCol = false;
>>
>> since make_foreignscan() already does that. But I will leave upto you to
>> remove this assignment or not.
>>
>
> As you pointed out, the assignment is redundant, but I think that that'd
> improve the clarity and readability. So, I'd vote for leaving that as is.
>

Ok. No problem.

>
> 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.
If we use rel->reltargetlist directly, without substituting nested loop
parameters, pull_var* coming across PlaceHolderVars or Vars from any
LATERAL or OUTER references. That won't create surprised now, but might do
that later. Second point is, the tlist passed to create_foreignscan_plan is
the one which gets projected during execution, so if we do not use it, we
might end up recording attributes different from the ones actually
projected by the node or required by quals.

>
> * I think that it'd improve the readability to match the code with other
> places that execute similar processing, such as check_index_only() and
> remove_unused_subquery_outputs().
>
>
Those functions are used during the path creation, so they don't have the
luxury of having ready-made tlist, hence use reltargetlist. But while in
planner, a ready-made tlist is available, so it better be used like all
create_*scan_plan() functions.

>
> Thanks,
>
> Best regards,
> Etsuro Fujita
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company