From 9612b982b8c9a8fd2c07e3c03e6a1878e4e41d9b Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 16 Apr 2014 17:22:00 -0400 Subject: [PATCH] Fix contrib/postgres_fdw's remote-estimate representation of array Params. We were emitting "(SELECT null::typename)", which is usually interpreted as a scalar subselect, but not so much in the context "x = ANY(...)". This led to remote-side parsing failures when remote_estimate is enabled. A quick and ugly fix is to stick in an extra cast step, "((SELECT null::typename)::typename)". The cast will be thrown away as redundant by parse analysis, but not before it's done its job of making sure the grammar sees the ANY argument as an a_expr rather than a select_with_parens. Per an example from Hannu Krosing. --- contrib/postgres_fdw/deparse.c | 84 ++++++++++++------- .../postgres_fdw/expected/postgres_fdw.out | 19 +++++ contrib/postgres_fdw/sql/postgres_fdw.sql | 3 + 3 files changed, 76 insertions(+), 30 deletions(-) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 696750c2354..28b13c5f0ac 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -131,6 +131,10 @@ static void deparseRelabelType(RelabelType *node, deparse_expr_cxt *context); static void deparseBoolExpr(BoolExpr *node, deparse_expr_cxt *context); static void deparseNullTest(NullTest *node, deparse_expr_cxt *context); static void deparseArrayExpr(ArrayExpr *node, deparse_expr_cxt *context); +static void printRemoteParam(int paramindex, Oid paramtype, int32 paramtypmod, + deparse_expr_cxt *context); +static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod, + deparse_expr_cxt *context); /* @@ -1272,16 +1276,11 @@ deparseVar(Var *node, deparse_expr_cxt *context) *context->params_list = lappend(*context->params_list, node); } - appendStringInfo(buf, "$%d", pindex); - appendStringInfo(buf, "::%s", - format_type_with_typemod(node->vartype, - node->vartypmod)); + printRemoteParam(pindex, node->vartype, node->vartypmod, context); } else { - appendStringInfo(buf, "(SELECT null::%s)", - format_type_with_typemod(node->vartype, - node->vartypmod)); + printRemotePlaceholder(node->vartype, node->vartypmod, context); } } } @@ -1388,26 +1387,12 @@ deparseConst(Const *node, deparse_expr_cxt *context) * * If we're generating the query "for real", add the Param to * context->params_list if it's not already present, and then use its index - * in that list as the remote parameter number. - * - * If we're just generating the query for EXPLAIN, replace the Param with - * a dummy expression "(SELECT null::)". In all extant versions of - * Postgres, the planner will see that as an unknown constant value, which is - * what we want. (If we sent a Param, recent versions might try to use the - * value supplied for the Param as an estimated or even constant value, which - * we don't want.) This might need adjustment if we ever make the planner - * flatten scalar subqueries. - * - * Note: we label the Param's type explicitly rather than relying on - * transmitting a numeric type OID in PQexecParams(). This allows us to - * avoid assuming that types have the same OIDs on the remote side as they - * do locally --- they need only have the same names. + * in that list as the remote parameter number. During EXPLAIN, there's + * no need to identify a parameter number. */ static void deparseParam(Param *node, deparse_expr_cxt *context) { - StringInfo buf = context->buf; - if (context->params_list) { int pindex = 0; @@ -1427,16 +1412,11 @@ deparseParam(Param *node, deparse_expr_cxt *context) *context->params_list = lappend(*context->params_list, node); } - appendStringInfo(buf, "$%d", pindex); - appendStringInfo(buf, "::%s", - format_type_with_typemod(node->paramtype, - node->paramtypmod)); + printRemoteParam(pindex, node->paramtype, node->paramtypmod, context); } else { - appendStringInfo(buf, "(SELECT null::%s)", - format_type_with_typemod(node->paramtype, - node->paramtypmod)); + printRemotePlaceholder(node->paramtype, node->paramtypmod, context); } } @@ -1813,3 +1793,47 @@ deparseArrayExpr(ArrayExpr *node, deparse_expr_cxt *context) appendStringInfo(buf, "::%s", format_type_with_typemod(node->array_typeid, -1)); } + +/* + * Print the representation of a parameter to be sent to the remote side. + * + * Note: we always label the Param's type explicitly rather than relying on + * transmitting a numeric type OID in PQexecParams(). This allows us to + * avoid assuming that types have the same OIDs on the remote side as they + * do locally --- they need only have the same names. + */ +static void +printRemoteParam(int paramindex, Oid paramtype, int32 paramtypmod, + deparse_expr_cxt *context) +{ + StringInfo buf = context->buf; + char *ptypename = format_type_with_typemod(paramtype, paramtypmod); + + appendStringInfo(buf, "$%d::%s", paramindex, ptypename); +} + +/* + * Print the representation of a placeholder for a parameter that will be + * sent to the remote side at execution time. + * + * This is used when we're just trying to EXPLAIN the remote query. + * We don't have the actual value of the runtime parameter yet, and we don't + * want the remote planner to generate a plan that depends on such a value + * anyway. Thus, we can't do something simple like "$1::paramtype". + * Instead, we emit "((SELECT null::paramtype)::paramtype)". + * In all extant versions of Postgres, the planner will see that as an unknown + * constant value, which is what we want. This might need adjustment if we + * ever make the planner flatten scalar subqueries. Note: the reason for the + * apparently useless outer cast is to ensure that the representation as a + * whole will be parsed as an a_expr and not a select_with_parens; the latter + * would do the wrong thing in the context "x = ANY(...)". + */ +static void +printRemotePlaceholder(Oid paramtype, int32 paramtypmod, + deparse_expr_cxt *context) +{ + StringInfo buf = context->buf; + char *ptypename = format_type_with_typemod(paramtype, paramtypmod); + + appendStringInfo(buf, "((SELECT null::%s)::%s)", ptypename, ptypename); +} diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 9a3d6516672..b6b05579e5f 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -592,6 +592,25 @@ WHERE a.c2 = 6 AND b.c1 = a.c1 AND a.c8 = 'foo' AND b.c7 = upper(a.c7); 996 | 6 | 00996 | Tue Apr 07 00:00:00 1970 PST | Tue Apr 07 00:00:00 1970 | 6 | 6 | foo | 996 | 6 | 00996 | Tue Apr 07 00:00:00 1970 PST | Tue Apr 07 00:00:00 1970 | 6 | 6 | foo (100 rows) +-- bug before 9.3.5 due to sloppy handling of remote-estimate parameters +SELECT * FROM ft1 WHERE c1 = ANY (ARRAY(SELECT c1 FROM ft2 WHERE c1 < 5)); + c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8 +----+----+-------+------------------------------+--------------------------+----+------------+----- + 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo + 2 | 2 | 00002 | Sat Jan 03 00:00:00 1970 PST | Sat Jan 03 00:00:00 1970 | 2 | 2 | foo + 3 | 3 | 00003 | Sun Jan 04 00:00:00 1970 PST | Sun Jan 04 00:00:00 1970 | 3 | 3 | foo + 4 | 4 | 00004 | Mon Jan 05 00:00:00 1970 PST | Mon Jan 05 00:00:00 1970 | 4 | 4 | foo +(4 rows) + +SELECT * FROM ft2 WHERE c1 = ANY (ARRAY(SELECT c1 FROM ft1 WHERE c1 < 5)); + c1 | c2 | c3 | c4 | c5 | c6 | c7 | c8 +----+----+-------+------------------------------+--------------------------+----+------------+----- + 1 | 1 | 00001 | Fri Jan 02 00:00:00 1970 PST | Fri Jan 02 00:00:00 1970 | 1 | 1 | foo + 2 | 2 | 00002 | Sat Jan 03 00:00:00 1970 PST | Sat Jan 03 00:00:00 1970 | 2 | 2 | foo + 3 | 3 | 00003 | Sun Jan 04 00:00:00 1970 PST | Sun Jan 04 00:00:00 1970 | 3 | 3 | foo + 4 | 4 | 00004 | Mon Jan 05 00:00:00 1970 PST | Mon Jan 05 00:00:00 1970 | 4 | 4 | foo +(4 rows) + -- =================================================================== -- parameterized queries -- =================================================================== diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 21b15ca9ff2..7ab3f74f337 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -200,6 +200,9 @@ EXPLAIN (VERBOSE, COSTS false) WHERE a.c2 = 6 AND b.c1 = a.c1 AND a.c8 = 'foo' AND b.c7 = upper(a.c7); SELECT * FROM ft2 a, ft2 b WHERE a.c2 = 6 AND b.c1 = a.c1 AND a.c8 = 'foo' AND b.c7 = upper(a.c7); +-- bug before 9.3.5 due to sloppy handling of remote-estimate parameters +SELECT * FROM ft1 WHERE c1 = ANY (ARRAY(SELECT c1 FROM ft2 WHERE c1 < 5)); +SELECT * FROM ft2 WHERE c1 = ANY (ARRAY(SELECT c1 FROM ft1 WHERE c1 < 5)); -- =================================================================== -- parameterized queries