Fix dumping of FUNCTION RTEs that contain non-function-call expressions.

The grammar will only accept something syntactically similar to a function
call in a function-in-FROM expression.  However, there are various ways
to input something that ruleutils.c won't deparse that way, potentially
leading to a view or rule that fails dump/reload.  Fix by inserting a
dummy CAST around anything that isn't going to deparse as a function
(which is one of the ways to get something like that in there in the
first place).

In HEAD, also make use of the infrastructure added by this to avoid
emitting unnecessary parentheses in CREATE INDEX deparsing.  I did
not change that in back branches, thinking that people might find it
to be unexpected/unnecessary behavioral change.

In HEAD, also fix incorrect logic for when to add extra parens to
partition key expressions.  Somebody apparently thought they could
get away with simpler logic than pg_get_indexdef_worker has, but
they were wrong --- a counterexample is PARTITION BY LIST ((a[1])).
Ignoring the prettyprint flag for partition expressions isn't exactly
a nice solution anyway.

This has been broken all along, so back-patch to all supported branches.

Discussion: https://postgr.es/m/10477.1499970459@sss.pgh.pa.us

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 5a4adbdc52e766ac9d6f2c06176f790419170731..5cfb916684f19d3c109be5d5ed4619f6e80c11cb 100644 (file)
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -416,6 +416,9 @@ static void get_rule_expr(Node *node, deparse_context *context,
              bool showimplicit);
 static void get_rule_expr_toplevel(Node *node, deparse_context *context,
                       bool showimplicit);
+static void get_rule_expr_funccall(Node *node, deparse_context *context,
+                      bool showimplicit);
+static bool looks_like_function(Node *node);
 static void get_oper_expr(OpExpr *expr, deparse_context *context);
 static void get_func_expr(FuncExpr *expr, deparse_context *context,
              bool showimplicit);
@@ -1308,8 +1311,7 @@ pg_get_indexdef_worker(Oid indexrelid, int colno,
            if (!colno || colno == keyno + 1)
            {
                /* Need parens if it's not a bare function call */
-               if (indexkey && IsA(indexkey, FuncExpr) &&
-                   ((FuncExpr *) indexkey)->funcformat == COERCE_EXPLICIT_CALL)
+               if (looks_like_function(indexkey))
                    appendStringInfoString(&buf, str);
                else
                    appendStringInfo(&buf, "(%s)", str);
@@ -1698,11 +1700,16 @@ pg_get_partkeydef_worker(Oid relid, int prettyFlags,
                elog(ERROR, "too few entries in partexprs list");
            partkey = (Node *) lfirst(partexpr_item);
            partexpr_item = lnext(partexpr_item);
+
            /* Deparse */
            str = deparse_expression_pretty(partkey, context, false, false,
-                                           0, 0);
+                                           prettyFlags, 0);
+           /* Need parens if it's not a bare function call */
+           if (looks_like_function(partkey))
+               appendStringInfoString(&buf, str);
+           else
+               appendStringInfo(&buf, "(%s)", str);
 
-           appendStringInfoString(&buf, str);
            keycoltype = exprType(partkey);
            keycolcollation = exprCollation(partkey);
        }
@@ -8776,6 +8783,64 @@ get_rule_expr_toplevel(Node *node, deparse_context *context,
        get_rule_expr(node, context, showimplicit);
 }
 
+/*
+ * get_rule_expr_funccall      - Parse back a function-call expression
+ *
+ * Same as get_rule_expr(), except that we guarantee that the output will
+ * look like a function call, or like one of the things the grammar treats as
+ * equivalent to a function call (see the func_expr_windowless production).
+ * This is needed in places where the grammar uses func_expr_windowless and
+ * you can't substitute a parenthesized a_expr.  If what we have isn't going
+ * to look like a function call, wrap it in a dummy CAST() expression, which
+ * will satisfy the grammar --- and, indeed, is likely what the user wrote to
+ * produce such a thing.
+ */
+static void
+get_rule_expr_funccall(Node *node, deparse_context *context,
+                      bool showimplicit)
+{
+   if (looks_like_function(node))
+       get_rule_expr(node, context, showimplicit);
+   else
+   {
+       StringInfo  buf = context->buf;
+
+       appendStringInfoString(buf, "CAST(");
+       /* no point in showing any top-level implicit cast */
+       get_rule_expr(node, context, false);
+       appendStringInfo(buf, " AS %s)",
+                        format_type_with_typemod(exprType(node),
+                                                 exprTypmod(node)));
+   }
+}
+
+/*
+ * Helper function to identify node types that satisfy func_expr_windowless.
+ * If in doubt, "false" is always a safe answer.
+ */
+static bool
+looks_like_function(Node *node)
+{
+   if (node == NULL)
+       return false;           /* probably shouldn't happen */
+   switch (nodeTag(node))
+   {
+       case T_FuncExpr:
+           /* OK, unless it's going to deparse as a cast */
+           return (((FuncExpr *) node)->funcformat == COERCE_EXPLICIT_CALL);
+       case T_NullIfExpr:
+       case T_CoalesceExpr:
+       case T_MinMaxExpr:
+       case T_SQLValueFunction:
+       case T_XmlExpr:
+           /* these are all accepted by func_expr_common_subexpr */
+           return true;
+       default:
+           break;
+   }
+   return false;
+}
+
 
 /*
  * get_oper_expr           - Parse back an OpExpr node
@@ -9749,7 +9814,7 @@ get_from_clause_item(Node *jtnode, Query *query, deparse_context *context)
                if (list_length(rte->functions) == 1 &&
                    (rtfunc1->funccolnames == NIL || !rte->funcordinality))
                {
-                   get_rule_expr(rtfunc1->funcexpr, context, true);
+                   get_rule_expr_funccall(rtfunc1->funcexpr, context, true);
                    /* we'll print the coldeflist below, if it has one */
                }
                else
@@ -9812,7 +9877,7 @@ get_from_clause_item(Node *jtnode, Query *query, deparse_context *context)
 
                            if (funcno > 0)
                                appendStringInfoString(buf, ", ");
-                           get_rule_expr(rtfunc->funcexpr, context, true);
+                           get_rule_expr_funccall(rtfunc->funcexpr, context, true);
                            if (rtfunc->funccolnames != NIL)
                            {
                                /* Reconstruct the column definition list */

diff --git a/src/test/regress/expected/create_table.out b/src/test/regress/expected/create_table.out
index b6f794e1c2aeae855b54c61d409bdfd9d33aa247..0b2f399cb155a3d7596b0eb0c2acc695460ae77d 100644 (file)
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -434,7 +434,7 @@ Partition key: RANGE (a oid_ops, plusone(b), c, d COLLATE "C")
  Column |  Type   | Collation | Nullable | Default 
 --------+---------+-----------+----------+---------
  a      | integer |           |          | 
-Partition key: LIST ((a + 1))
+Partition key: LIST (((a + 1)))
 
 DROP TABLE partitioned, partitioned2;
 --

diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index c719262720eb4053a77917e0647add772ab425af..79f5dba55fc98109ff2ff2c9e171f8829b22acce 100644 (file)
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -1622,6 +1622,32 @@ select pg_get_viewdef('tt19v', true);
      'foo'::text = ANY ((( SELECT ARRAY['abc'::text, 'def'::text, 'foo'::text] AS "array"))::text[]) AS c2;
 (1 row)
 
+-- check display of assorted RTE_FUNCTION expressions
+create view tt20v as
+select * from
+  coalesce(1,2) as c,
+  collation for ('x'::text) col,
+  current_date as d,
+  localtimestamp(3) as t,
+  cast(1+2 as int4) as i4,
+  cast(1+2 as int8) as i8;
+select pg_get_viewdef('tt20v', true);
+               pg_get_viewdef                
+---------------------------------------------
+  SELECT c.c,                               +
+     col.col,                               +
+     d.d,                                   +
+     t.t,                                   +
+     i4.i4,                                 +
+     i8.i8                                  +
+    FROM COALESCE(1, 2) c(c),               +
+     pg_collation_for('x'::text) col(col),  +
+     CURRENT_DATE d(d),                     +
+     LOCALTIMESTAMP(3) t(t),                +
+     CAST(1 + 2 AS integer) i4(i4),         +
+     CAST((1 + 2)::bigint AS bigint) i8(i8);
+(1 row)
+
 -- clean up all the random objects we made above
 set client_min_messages = warning;
 DROP SCHEMA temp_view_test CASCADE;

diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index d6f50d6105f6c66ae83d352e40f97b3189cb5cd8..85c2959d3fd30fe239d9dacb0437f02da7fe3623 100644 (file)
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -547,6 +547,18 @@ select 'foo'::text = any(array['abc','def','foo']::text[]) c1,
        'foo'::text = any((select array['abc','def','foo']::text[])::text[]) c2;
 select pg_get_viewdef('tt19v', true);
 
+-- check display of assorted RTE_FUNCTION expressions
+
+create view tt20v as
+select * from
+  coalesce(1,2) as c,
+  collation for ('x'::text) col,
+  current_date as d,
+  localtimestamp(3) as t,
+  cast(1+2 as int4) as i4,
+  cast(1+2 as int8) as i8;
+select pg_get_viewdef('tt20v', true);
+
 -- clean up all the random objects we made above
 set client_min_messages = warning;
 DROP SCHEMA temp_view_test CASCADE;