Re: Minor code improvements to create_foreignscan_plan/ExecInitForeignScan

On Tue, Dec 22, 2015 at 7:32 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Sep 9, 2015 at 7:08 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> On 2015/07/10 18:40, Etsuro Fujita 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.
>>
>> Attached is an updated version of the patch. The previous version
>> contained changes to ExecInitForeignScan, but I've dropped that part, as
>> discussed before.
>
> Moved to next CF because of a lack of reviews.

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:

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.

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

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company