Fix coerce_to_target_type for coerce_type's klugy handling of COLLATE.

Because coerce_type recurses into the argument of a CollateExpr,
coerce_to_target_type's longstanding code for detecting whether coerce_type
had actually done anything (to wit, returned a different node than it
passed in) was broken in 9.1.  This resulted in unexpected failures in
hide_coercion_node; which was not the latter's fault, since it's critical
that we never call it on anything that wasn't inserted by coerce_type.
(Else we might decide to "hide" a user-written function call.)

Fix by removing and replacing the CollateExpr in coerce_to_target_type
itself.  This is all pretty ugly but I don't immediately see a way to make
it nicer.

Per report from Jean-Yves F. Barbier.

diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index e72783745449f942cf9e8c428096a73bd2e61e2c..6661a3d8a1d1044eccdef2e3c2e7bf2fd59a951d 100644 (file)
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -79,10 +79,24 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
                                          int location)
 {
        Node       *result;
+       Node       *origexpr;
 
        if (!can_coerce_type(1, &exprtype, &targettype, ccontext))
                return NULL;
 
+       /*
+        * If the input has a CollateExpr at the top, strip it off, perform the
+        * coercion, and put a new one back on.  This is annoying since it
+        * duplicates logic in coerce_type, but if we don't do this then it's too
+        * hard to tell whether coerce_type actually changed anything, and we
+        * *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.
+        */
+       origexpr = expr;
+       while (expr && IsA(expr, CollateExpr))
+               expr = (Node *) ((CollateExpr *) expr)->arg;
+
        result = coerce_type(pstate, expr, exprtype,
                                                 targettype, targettypmod,
                                                 ccontext, cformat, location);
@@ -98,6 +112,18 @@ coerce_to_target_type(ParseState *pstate, Node *expr, Oid exprtype,
                                                                (cformat != COERCE_IMPLICIT_CAST),
                                                                (result != expr && !IsA(result, Const)));
 
+       if (expr != origexpr)
+       {
+               /* Reinstall top CollateExpr */
+               CollateExpr *coll = (CollateExpr *) origexpr;
+               CollateExpr *newcoll = makeNode(CollateExpr);
+
+               newcoll->arg = (Expr *) result;
+               newcoll->collOid = coll->collOid;
+               newcoll->location = coll->location;
+               result = (Node *) newcoll;
+       }
+
        return result;
 }
 
@@ -318,7 +344,7 @@ 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
-                * otherwise.
+                * otherwise.  This kluge has consequences in coerce_to_target_type.
                 */
                CollateExpr *coll = (CollateExpr *) node;
                CollateExpr *newcoll = makeNode(CollateExpr);

diff --git a/src/test/regress/expected/collate.out b/src/test/regress/expected/collate.out
index dc17feaefe41e2ead7e69ea8a9bcccdc06a4670e..a15e6911b0af98c92854b6ecdcc6437adf89f824 100644 (file)
--- a/src/test/regress/expected/collate.out
+++ b/src/test/regress/expected/collate.out
@@ -574,6 +574,9 @@ ALTER TABLE collate_test22 ADD FOREIGN KEY (f2) REFERENCES collate_test20;
 RESET enable_seqscan;
 RESET enable_hashjoin;
 RESET enable_nestloop;
+-- 9.1 bug with useless COLLATE in an expression subject to length coercion
+CREATE TEMP TABLE vctable (f1 varchar(25));
+INSERT INTO vctable VALUES ('foo' COLLATE "C");
 --
 -- 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

diff --git a/src/test/regress/sql/collate.sql b/src/test/regress/sql/collate.sql
index 52d830d58a976282cda1c7246ec06f45dce36e3c..f72f3edb9d231ff42267da3f44e8421abcccfbcd 100644 (file)
--- a/src/test/regress/sql/collate.sql
+++ b/src/test/regress/sql/collate.sql
@@ -214,6 +214,11 @@ RESET enable_seqscan;
 RESET enable_hashjoin;
 RESET enable_nestloop;
 
+-- 9.1 bug with useless COLLATE in an expression subject to length coercion
+
+CREATE TEMP TABLE vctable (f1 varchar(25));
+INSERT INTO vctable VALUES ('foo' COLLATE "C");
+
 --
 -- 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