Fix old bug with coercing the result of a COLLATE expression.

There are hacks in parse_coerce.c to push down a requested coercion
to below any CollateExpr that may appear.  However, we did that even
if the requested data type is non-collatable, leading to an invalid
expression tree in which CollateExpr is applied to a non-collatable
type.  The fix is just to drop the CollateExpr altogether, reasoning
that it's useless.

This bug is ten years old, dating to the original addition of
COLLATE support.  The lack of field complaints suggests that there
aren't a lot of user-visible consequences.  We noticed the problem
because it would trigger an assertion in DefineVirtualRelation if
the invalid structure appears as an output column of a view; however,
in a non-assert build, you don't see a crash just a (subtly incorrect)
complaint about applying collation to a non-collatable type.  I found
that by putting the incorrect structure further down in a view, I could
make a view definition that would fail dump/reload, per the added
regression test case.  But CollateExpr doesn't do anything at run-time,
so this likely doesn't lead to any really exciting consequences.

Per report from Yulin Pei.  Back-patch to all supported branches.

Discussion: https://postgr.es/m/HK0PR01MB22744393C474D503E16C8509F4709@HK0PR01MB2274.apcprd01.prod.exchangelabs.com

diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 065535a26bb54f5a8ac5eece076a2e1fe053bfd2..f255e005d4012a38e1afcd914b38a15d0db63036 100644 (file)
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -95,6 +95,7 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
     * *must* know that to avoid possibly calling hide_coercion_node on
     * something that wasn't generated by coerce_type.  Note that if there are
     * multiple stacked CollateExprs, we just discard all but the topmost.
+    * Also, if the target type isn't collatable, we discard the CollateExpr.
     */
    origexpr = expr;
    while (expr && IsA(expr, CollateExpr))
@@ -114,7 +115,7 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
                                ccontext, cformat, location,
                                (result != expr && !IsA(result, Const)));
 
-   if (expr != origexpr)
+   if (expr != origexpr && type_is_collatable(targettype))
    {
        /* Reinstall top CollateExpr */
        CollateExpr *coll = (CollateExpr *) origexpr;
@@ -382,20 +383,26 @@ coerce_type(ParseState *pstate, Node *node,
    {
        /*
         * If we have a COLLATE clause, we have to push the coercion
-        * underneath the COLLATE.  This is really ugly, but there is little
-        * choice because the above hacks on Consts and Params wouldn't happen
+        * underneath the COLLATE; or discard the COLLATE if the target type
+        * isn't collatable.  This is really ugly, but there is little choice
+        * because the above hacks on Consts and Params wouldn't happen
         * otherwise.  This kluge has consequences in coerce_to_target_type.
         */
        CollateExpr *coll = (CollateExpr *) node;
-       CollateExpr *newcoll = makeNode(CollateExpr);
 
-       newcoll->arg = (Expr *)
-           coerce_type(pstate, (Node *) coll->arg,
-                       inputTypeId, targetTypeId, targetTypeMod,
-                       ccontext, cformat, location);
-       newcoll->collOid = coll->collOid;
-       newcoll->location = coll->location;
-       return (Node *) newcoll;
+       result = coerce_type(pstate, (Node *) coll->arg,
+                            inputTypeId, targetTypeId, targetTypeMod,
+                            ccontext, cformat, location);
+       if (type_is_collatable(targetTypeId))
+       {
+           CollateExpr *newcoll = makeNode(CollateExpr);
+
+           newcoll->arg = (Expr *) result;
+           newcoll->collOid = coll->collOid;
+           newcoll->location = coll->location;
+           result = (Node *) newcoll;
+       }
+       return result;
    }
    pathtype = find_coercion_pathway(targetTypeId, inputTypeId, ccontext,
                                     &funcId);

diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
index fcbe3a5cc8234558bf60d28866fa310e62112d15..7b52dcd626ac2b156b07b3bb4397f4383b60a13f 100644 (file)
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -661,6 +661,19 @@ SELECT collation for ((SELECT b FROM collate_test1 LIMIT 1));
  "C"
 (1 row)
 
+-- old bug with not dropping COLLATE when coercing to non-collatable type
+CREATE VIEW collate_on_int AS
+SELECT c1+1 AS c1p FROM
+  (SELECT ('4' COLLATE "C")::INT AS c1) ss;
+\d+ collate_on_int
+                    View "collate_tests.collate_on_int"
+ Column |  Type   | Collation | Nullable | Default | Storage | Description 
+--------+---------+-----------+----------+---------+---------+-------------
+ c1p    | integer |           |          |         | plain   | 
+View definition:
+ SELECT ss.c1 + 1 AS c1p
+   FROM ( SELECT 4 AS c1) ss;
+
 --
 -- Clean up.  Many of these table names will be re-used if the user is
 -- trying to run any platform-specific collation tests later, so we
@@ -668,4 +681,4 @@ SELECT collation for ((SELECT b FROM collate_test1 LIMIT 1));
 --
 \set VERBOSITY terse
 DROP SCHEMA collate_tests CASCADE;
-NOTICE:  drop cascades to 17 other objects
+NOTICE:  drop cascades to 18 other objects

diff --git a/src/test/regress/sql/collate.sql b/src/test/regress/sql/collate.sql
index 4ddde95a5e8efed5ea9a07ba9c18a1610ec9eb13..30370d61a27a13bb2a774c812486be54ccd093a8 100644 (file)
--- a/src/test/regress/sql/collate.sql
+++ b/src/test/regress/sql/collate.sql
@@ -253,6 +253,12 @@ SELECT collation for ('foo'::text);
 SELECT collation for ((SELECT a FROM collate_test1 LIMIT 1)); -- non-collatable type - error
 SELECT collation for ((SELECT b FROM collate_test1 LIMIT 1));
 
+-- old bug with not dropping COLLATE when coercing to non-collatable type
+CREATE VIEW collate_on_int AS
+SELECT c1+1 AS c1p FROM
+  (SELECT ('4' COLLATE "C")::INT AS c1) ss;
+\d+ collate_on_int
+
 
 --
 -- Clean up.  Many of these table names will be re-used if the user is