Re: Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

On 2015/07/10 21:59, David Rowley wrote:
> On 10 July 2015 at 21:40, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp
> <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
>
>     To save cycles, I modified create_foreignscan_plan so that it detects
>     whether any system columns are requested if scanning a base relation.
>     Also, I revised other code there a little bit.
>
>     For ExecInitForeignScan, I simplified the code there to determine the
>     scan tuple type, whith seems to me complex beyound necessity.  Maybe
>     that might be nitpicking, though.

For the latter, I misunderstood that the latest foreign-join pushdown 
patch allows fdw_scan_tlist to be set to a targetlist even for simple 
foreign table scans.  Sorry for that, but I have a concern about that. 
I'd like to mention it in a new thread later.

> 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.

Good catch!

Will update the patch (and drop the ExecInitForeignScan part from the 
patch).

Sorry for the delay.

Best regards,
Etsuro Fujita