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(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp
> <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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