(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.
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.
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.
Please find attached an updated version of the patch.
Thanks,
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***************
*** 253,258 **** foreign_expr_walker(Node *node,
--- 253,268 ----
if (var->varno == glob_cxt->foreignrel->relid &&
var->varlevelsup == 0)
{
+ /*
+ * System columns can't be sent to remote since the values
+ * for system columns might be different between local and
+ * remote.
+ *
+ * XXX: we should probably send ctid to remote.
+ */
+ if (var->varattno < 0)
+ return false;
+
/* Var belongs to foreign table */
collation = var->varcollid;
state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
***************
*** 1262,1267 **** deparseVar(Var *node, deparse_expr_cxt *context)
--- 1272,1280 ----
if (node->varno == context->foreignrel->relid &&
node->varlevelsup == 0)
{
+ /* Var shouldn't be a system column */
+ Assert(node->varattno >= 0);
+
/* Var belongs to foreign table */
deparseColumnRef(buf, node->varno, node->varattno, context->root);
}
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 353,358 **** EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = t1.c2;
--- 353,368 ----
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" = c2))
(3 rows)
+ -- where with system column conditions
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.tableoid != 0;
+ QUERY PLAN
+ -------------------------------------------------------------------------
+ Foreign Scan on public.ft1 t1
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ Filter: (t1.tableoid <> 0::oid)
+ Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
+ (4 rows)
+
-- ===================================================================
-- WHERE with remotely-executable conditions
-- ===================================================================
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 180,185 **** EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = postgres_fdw_a
--- 180,187 ----
EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 === t1.c2;
EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = abs(t1.c2);
EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = t1.c2;
+ -- where with system column conditions
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.tableoid != 0;
-- ===================================================================
-- WHERE with remotely-executable conditions
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
