On Tue, Nov 18, 2014 at 1:55 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote:
> (2014/11/17 19:54), Ashutosh Bapat wrote: > >> Here are comments for postgres_fdw-syscol patch. >> > > Thanks for the review! > > 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. >> > > Done. > > I would add " and foreign values do not make sense locally (except may be ctid clubbed with foreign server_id)" to make it more clear. But I will leave that for the commiter to decide. > 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. >> > > Done. > Thanks > > 3. Since there is already a testcase which triggered this particular >> change, it will good, if we add that to regression in postgres_fdw. >> > > Done. > > I think, a better case would be SELECT * FROM ft1 t1, pg_class t2 WHERE t1.tableoid = t2.oid. The condition makes sure that the tableoid in the row is same as the OID of the foreign table recorded in pg_class locally. And the EXPLAIN of the query which clearly shows that the tableoid column in not fetched from the foreign server. Please find attached an updated version of the patch. Once we resolve the other patch on this thread, I think this item can be marked as ready for commiter from my side. > > > Thanks, > > Best regards, > Etsuro Fujita > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
