Re: postgres_fdw behaves oddly

Hi Fujita-san,

Here are comments for postgres_fdw-syscol patch.

Sanity
--------
The patch applies and compiles cleanly.
The server regression and regression in contrib/postgres_fdw,file_fdw run
cleanly.

Code
-------
1. Instead of a single liner comment "System columns can't be sent to
remote.", it might be better to explain why system columns can't be sent to
the remote.
2. The comment in deparseVar is single line comment, so it should start and
end on the same line i.e. /* and */ should be on the same line.
3. Since there is already a testcase which triggered this particular
change, it will good, if we add that to regression in postgres_fdw.

On Mon, Nov 17, 2014 at 4:06 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> Hi Fujita-san,
>
> Here are my comments about the patch fscan_reltargetlist.patch
> Sanity
> --------
> Patch applies and compiles cleanly.
> Regressions in test/regress folder and postgres_fdw and file_fdw are clean.
>
> 1. This isn't your change, but we might be able to get rid of assignment
> 2071 /* Are any system columns requested from rel? */
> 2072 scan_plan->fsSystemCol = false;
>
> since make_foreignscan() already does that. But I will leave upto you to
> remove this assignment or not.
>
> 2. Instead of using rel->reltargetlist, we should use the tlist passed by
> caller. This is the tlist expected from the Plan node. For foreign scans it
> will be same as rel->reltargetlist. Using tlist would make the function
> consistent with create_*scan_plan functions.
>
> Otherwise, the patch looks good.
>
> On Wed, Nov 12, 2014 at 12:55 PM, Etsuro Fujita <
> fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
>> (2014/11/11 18:45), Etsuro Fujita wrote:
>>
>>> (2014/11/10 20:05), Ashutosh Bapat wrote:
>>>
>>>> Since two separate issues 1. using reltargetlist instead of attr_needed
>>>> and 2. system columns usage in FDW are being tackled here, we should
>>>> separate the patch into two one for each of the issues.
>>>>
>>>
>>> Agreed. Will split the patch into two.
>>>
>>
>> Here are splitted patches:
>>
>> fscan-reltargetlist.patch - patch for #1
>> postgres_fdw-syscol.patch - patch for #2
>>
>>
>> Thanks,
>>
>> Best regards,
>> Etsuro Fujita
>>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company