Re: Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

On 2016/01/15 19:00, Etsuro Fujita wrote:
> 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.

On second thought, I noticed that detecting whether we see a system 
column that way needs more cycles in cases where the reltargetlist and 
the restriction clauses don't contain any system columns.  ISTM that 
such cases are rather common, so I'm inclined to keep that code as-is.

Best regards,
Etsuro Fujita