Re: Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

I wonder,

> --- 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;
> }

Would it make sense to call pull_varattnos(reltargetlist), then walk the
bitmapset and break if we see a system column, then call
pull_varattnos() on the rinfo->clause? That way, if the targetlist
request a system column we don't have to walk the RestrictInfos.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services