Fix contrib/postgres_fdw's remote-estimate representation of array Params.
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 16 Apr 2014 21:21:57 +0000 (17:21 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 16 Apr 2014 21:21:57 +0000 (17:21 -0400)
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
contrib/postgres_fdw/expected/postgres_fdw.out
contrib/postgres_fdw/sql/postgres_fdw.sql

index e54d46d62b18441bb1dd4c92d3a5df08b2674391..4e6fa8b805e8f9944c5cffd237dfbc1997865b89 100644 (file)
@@ -132,6 +132,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);
 
 
 /*
@@ -1283,16 +1287,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);
        }
    }
 }
@@ -1399,26 +1398,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::<type>)". 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;
@@ -1438,16 +1423,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);
    }
 }
 
@@ -1816,3 +1796,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);
+}
index eeb0b0f291c5a5a6ac56e32d9ea4bc33c1c8d8f5..2e49ee317a2901af6107b5e4f8bd940c392b9fa4 100644 (file)
@@ -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
 -- ===================================================================
index e11f7d9960259d14b159ede63927d3468762c37f..6187839453c0aa6cf30e561773e1e84d3b4d4e29 100644 (file)
@@ -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