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 f26c69abddd83f2687df6029379441644070a8a9..bfd379323a5272cf93259d91634211280a29f5ab 100644 (file)
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -81,10 +81,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);
@@ -100,6 +114,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