Postgres Pro
Downloads
Home   >   mailing lists

Re: postgres_fdw behaves oddly - Mailing list pgsql-hackers

From Ashutosh Bapat
Subject Re: postgres_fdw behaves oddly
Date
Msg-id CAFjFpRfmbGM3Cvq2eD-WK=tN3DxHymBitEU=njQOyzrq6eQggA@mail.gmail.com
Whole thread Raw
In response to Re: postgres_fdw behaves oddly  (Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>)
Responses Re: postgres_fdw behaves oddly  (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
List pgsql-hackers
Tree view
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.bapat@enterprisedb.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.etsuro@lab.ntt.co.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

pgsql-hackers by date:

Previous
From: Ashutosh Bapat
Date:
Subject: Re: postgres_fdw behaves oddly
Next
From: Heikki Linnakangas
Date:
Subject: Re: WAL format and API changes (9.5)