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