On 2015/12/23 2:47, Robert Haas wrote:
On Tue, Dec 22, 2015 at 7:32 AM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
Moved to next CF because of a lack of reviews.
Thanks, Michael!
I just took a look at this. I think the basic idea of this patch is
good, but the comments need some work, because they don't really
explain why this should be skipped in the join case. Maybe something
like this:
Thanks for the review, Robert!
If rel is a base relation, detect whether any system columns were
requested. (If rel is a join relation, rel->relid will be 0, but
there can be no Var in the target list with relid 0, so we skip this
in that case.) This is a bit of a kluge and might go away someday, so
we intentionally leave it out of the API presented to FDWs.
And the rest as it is currently written.
Agreed.
It might be good, also, to say something about why we never need
fsSystemCol to be true in the joinrel case.
+1 for that. How about adding something like this:
Note that any such system columns are assumed to be contained in
fdw_scan_tlist, so we never need fsSystemCol to be true in the joinrel case.
Attached is an updated version of the patch.
Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***************
*** 2097,2106 **** create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
RelOptInfo *rel = best_path->path.parent;
Index scan_relid = rel->relid;
Oid rel_oid = InvalidOid;
- Bitmapset *attrs_used = NULL;
Plan *outer_plan = NULL;
- ListCell *lc;
- int i;
Assert(rel->fdwroutine != NULL);
--- 2097,2103 ----
***************
*** 2169,2204 **** create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
}
/*
! * Detect whether any system columns are requested from rel. This is a
! * bit of a kluge and might go away someday, so we intentionally leave it
! * out of the API presented to FDWs.
! *
! * First, examine all the attributes needed for joins or final output.
! * Note: we must look at reltargetlist, not the attr_needed data, because
! * attr_needed isn't computed for inheritance child rels.
*/
! pull_varattnos((Node *) rel->reltargetlist, rel->relid, &attrs_used);
!
! /* Add all the attributes used by restriction clauses. */
! foreach(lc, rel->baserestrictinfo)
{
! RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
! pull_varattnos((Node *) rinfo->clause, rel->relid, &attrs_used);
! }
! /* Now, are any system columns requested from rel? */
! scan_plan->fsSystemCol = false;
! for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! {
! if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
{
! scan_plan->fsSystemCol = true;
! break;
}
- }
! bms_free(attrs_used);
return scan_plan;
}
--- 2166,2213 ----
}
/*
! * If rel is a base relation, detect whether any system columns are
! * requested from the rel. (If rel is a join relation, rel->relid will be
! * 0, but there can be no Var in the target list with relid 0, so we skip
! * this in that case. Note that any such system columns are assumed to be
! * contained in fdw_scan_tlist, so we never need fsSystemCol to be true in
! * the joinrel case.) This is a bit of a kluge and might go away someday,
! * so we intentionally leave it out of the API presented to FDWs.
*/
! scan_plan->fsSystemCol = false;
! if (scan_relid > 0)
{
! Bitmapset *attrs_used = NULL;
! ListCell *lc;
! int i;
! /*
! * First, examine all the attributes needed for joins or final output.
! * Note: we must look at reltargetlist, not the attr_needed data,
! * because attr_needed isn't computed for inheritance child rels.
! */
! pull_varattnos((Node *) rel->reltargetlist, scan_relid, &attrs_used);
! /* Add all the attributes used by restriction clauses. */
! foreach(lc, rel->baserestrictinfo)
{
! RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
!
! pull_varattnos((Node *) rinfo->clause, scan_relid, &attrs_used);
}
! /* Now, are any system columns requested from rel? */
! for (i = FirstLowInvalidHeapAttributeNumber + 1; i < 0; i++)
! {
! if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
! {
! scan_plan->fsSystemCol = true;
! break;
! }
! }
!
! bms_free(attrs_used);
! }
return scan_plan;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
