Re: Minor code improvements to create_foreignscan_plan/ExecInitForeignScan - Mailing list pgsql-hackers

On 2015/07/22 15:25, Etsuro Fujita wrote:

On 2015/07/10 21:59, David Rowley wrote:

I just glanced at this and noticed that the method for determining if
there's any system columns could be made a bit nicer.

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

I think could just be written as:
/* Now, are any system columns requested from rel? */
if (!bms_is_empty(attrs_used) &&
bms_next_member(attrs_used, -1) < -FirstLowInvalidHeapAttributeNumber)
scan_plan->fsSystemCol = true;
else
scan_plan->fsSystemCol = false;

I know you didn't change this, but just thought I'd mention it while
there's an opportunity to fix it.

On second thought, I noticed that there is a case when that fix doesn't work well; bms_next_member wouldn't be efficient when only the rear user-columns are requested from a foreign table that has a large number of user-columns.  So, I'm inclined to leave that as-is.