Re: Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

On 2016/01/12 18:00, Etsuro Fujita wrote:
> On 2016/01/12 2:36, Alvaro Herrera wrote:
>> 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.

> Seems like a good idea. Will update the patch.

Done. Attached is an updated version of the patch.

Best regards,
Etsuro Fujita