Re: Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

On 2015/12/23 2:47, Robert Haas wrote:
> On Tue, Dec 22, 2015 at 7:32 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Moved to next CF because of a lack of reviews.

Thanks, Michael!

> I just took a look at this.  I think the basic idea of this patch is
> good, but the comments need some work, because they don't really
> explain why this should be skipped in the join case.  Maybe something
> like this:

Thanks for the review, Robert!

> If rel is a base relation, detect whether any system columns were
> requested.  (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.) This is a bit of a kluge and might go away someday, so
> we intentionally leave it out of the API presented to FDWs.
> And the rest as it is currently written.

Agreed.

> It might be good, also, to say something about why we never need
> fsSystemCol to be true in the joinrel case.

+1 for that.  How about adding something like this:

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.

Attached is an updated version of the patch.

Best regards,
Etsuro Fujita