Re: postgres_fdw behaves oddly - Mailing list pgsql-hackers
| From | Ashutosh Bapat |
|---|---|
| Subject | Re: postgres_fdw behaves oddly |
| Date | |
| Msg-id | CAFjFpRd4v=xqEFNYUJAv4m9ZEroG_-UB1cQ3FOrRtJ=eC8=W-Q@mail.gmail.com Whole thread Raw |
| In response to | Re: postgres_fdw behaves oddly (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>) |
| Responses |
Re: postgres_fdw behaves oddly
(Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
|
| List | pgsql-hackers |
On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
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.
(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;
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
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
pgsql-hackers by date: