Make equal() ignore CoercionForm fields for better planning with casts.

This change ensures that the planner will see implicit and explicit casts
as equivalent for all purposes, except in the minority of cases where
there's actually a semantic difference (as reflected by having a 3-argument
cast function).  In particular, this fixes cases where the EquivalenceClass
machinery failed to consider two references to a varchar column as
equivalent if one was implicitly cast to text but the other was explicitly
cast to text, as seen in bug #7598 from Vaclav Juza.  We have had similar
bugs before in other parts of the planner, so I think it's time to fix this
problem at the core instead of continuing to band-aid around it.

Remove set_coercionform_dontcare(), which represents the band-aid
previously in use for allowing matching of index and constraint expressions
with inconsistent cast labeling.  (We can probably get rid of
COERCE_DONTCARE altogether, but I don't think removing that enum value in
back branches would be wise; it's possible there's third party code
referring to it.)

Back-patch to 9.2.  We could go back further, and might want to once this
has been tested more; but for the moment I won't risk destabilizing plan
choices in long-since-stable branches.

diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 2171d8d018bab95e0ed118384ddf8416502a86e8..40fd80fd9cdfa8fb9049fd233a971805dc11a1df 100644 (file)
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -83,6 +83,10 @@
 #define COMPARE_LOCATION_FIELD(fldname) \
    ((void) 0)
 
+/* Compare a CoercionForm field (also a no-op, per comment in primnodes.h) */
+#define COMPARE_COERCIONFORM_FIELD(fldname) \
+   ((void) 0)
+
 
 /*
  * Stuff from primnodes.h
@@ -235,16 +239,7 @@ _equalFuncExpr(const FuncExpr *a, const FuncExpr *b)
    COMPARE_SCALAR_FIELD(funcid);
    COMPARE_SCALAR_FIELD(funcresulttype);
    COMPARE_SCALAR_FIELD(funcretset);
-
-   /*
-    * Special-case COERCE_DONTCARE, so that planner can build coercion nodes
-    * that are equal() to both explicit and implicit coercions.
-    */
-   if (a->funcformat != b->funcformat &&
-       a->funcformat != COERCE_DONTCARE &&
-       b->funcformat != COERCE_DONTCARE)
-       return false;
-
+   COMPARE_COERCIONFORM_FIELD(funcformat);
    COMPARE_SCALAR_FIELD(funccollid);
    COMPARE_SCALAR_FIELD(inputcollid);
    COMPARE_NODE_FIELD(args);
@@ -448,16 +443,7 @@ _equalRelabelType(const RelabelType *a, const RelabelType *b)
    COMPARE_SCALAR_FIELD(resulttype);
    COMPARE_SCALAR_FIELD(resulttypmod);
    COMPARE_SCALAR_FIELD(resultcollid);
-
-   /*
-    * Special-case COERCE_DONTCARE, so that planner can build coercion nodes
-    * that are equal() to both explicit and implicit coercions.
-    */
-   if (a->relabelformat != b->relabelformat &&
-       a->relabelformat != COERCE_DONTCARE &&
-       b->relabelformat != COERCE_DONTCARE)
-       return false;
-
+   COMPARE_COERCIONFORM_FIELD(relabelformat);
    COMPARE_LOCATION_FIELD(location);
 
    return true;
@@ -469,16 +455,7 @@ _equalCoerceViaIO(const CoerceViaIO *a, const CoerceViaIO *b)
    COMPARE_NODE_FIELD(arg);
    COMPARE_SCALAR_FIELD(resulttype);
    COMPARE_SCALAR_FIELD(resultcollid);
-
-   /*
-    * Special-case COERCE_DONTCARE, so that planner can build coercion nodes
-    * that are equal() to both explicit and implicit coercions.
-    */
-   if (a->coerceformat != b->coerceformat &&
-       a->coerceformat != COERCE_DONTCARE &&
-       b->coerceformat != COERCE_DONTCARE)
-       return false;
-
+   COMPARE_COERCIONFORM_FIELD(coerceformat);
    COMPARE_LOCATION_FIELD(location);
 
    return true;
@@ -493,16 +470,7 @@ _equalArrayCoerceExpr(const ArrayCoerceExpr *a, const ArrayCoerceExpr *b)
    COMPARE_SCALAR_FIELD(resulttypmod);
    COMPARE_SCALAR_FIELD(resultcollid);
    COMPARE_SCALAR_FIELD(isExplicit);
-
-   /*
-    * Special-case COERCE_DONTCARE, so that planner can build coercion nodes
-    * that are equal() to both explicit and implicit coercions.
-    */
-   if (a->coerceformat != b->coerceformat &&
-       a->coerceformat != COERCE_DONTCARE &&
-       b->coerceformat != COERCE_DONTCARE)
-       return false;
-
+   COMPARE_COERCIONFORM_FIELD(coerceformat);
    COMPARE_LOCATION_FIELD(location);
 
    return true;
@@ -513,16 +481,7 @@ _equalConvertRowtypeExpr(const ConvertRowtypeExpr *a, const ConvertRowtypeExpr *
 {
    COMPARE_NODE_FIELD(arg);
    COMPARE_SCALAR_FIELD(resulttype);
-
-   /*
-    * Special-case COERCE_DONTCARE, so that planner can build coercion nodes
-    * that are equal() to both explicit and implicit coercions.
-    */
-   if (a->convertformat != b->convertformat &&
-       a->convertformat != COERCE_DONTCARE &&
-       b->convertformat != COERCE_DONTCARE)
-       return false;
-
+   COMPARE_COERCIONFORM_FIELD(convertformat);
    COMPARE_LOCATION_FIELD(location);
 
    return true;
@@ -589,16 +548,7 @@ _equalRowExpr(const RowExpr *a, const RowExpr *b)
 {
    COMPARE_NODE_FIELD(args);
    COMPARE_SCALAR_FIELD(row_typeid);
-
-   /*
-    * Special-case COERCE_DONTCARE, so that planner can build coercion nodes
-    * that are equal() to both explicit and implicit coercions.
-    */
-   if (a->row_format != b->row_format &&
-       a->row_format != COERCE_DONTCARE &&
-       b->row_format != COERCE_DONTCARE)
-       return false;
-
+   COMPARE_COERCIONFORM_FIELD(row_format);
    COMPARE_NODE_FIELD(colnames);
    COMPARE_LOCATION_FIELD(location);
 
@@ -684,16 +634,7 @@ _equalCoerceToDomain(const CoerceToDomain *a, const CoerceToDomain *b)
    COMPARE_SCALAR_FIELD(resulttype);
    COMPARE_SCALAR_FIELD(resulttypmod);
    COMPARE_SCALAR_FIELD(resultcollid);
-
-   /*
-    * Special-case COERCE_DONTCARE, so that planner can build coercion nodes
-    * that are equal() to both explicit and implicit coercions.
-    */
-   if (a->coercionformat != b->coercionformat &&
-       a->coercionformat != COERCE_DONTCARE &&
-       b->coercionformat != COERCE_DONTCARE)
-       return false;
-
+   COMPARE_COERCIONFORM_FIELD(coercionformat);
    COMPARE_LOCATION_FIELD(location);
 
    return true;

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index c2d36ca38d8d98d2a307cd3d2a5143a9545665ef..49f039138766b0b8cb99cb8bd57f3dfe8a992dd3 100644 (file)
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -97,7 +97,6 @@ static bool contain_leaky_functions_walker(Node *node, void *context);
 static Relids find_nonnullable_rels_walker(Node *node, bool top_level);
 static List *find_nonnullable_vars_walker(Node *node, bool top_level);
 static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
-static bool set_coercionform_dontcare_walker(Node *node, void *context);
 static Node *eval_const_expressions_mutator(Node *node,
                               eval_const_expressions_context *context);
 static List *simplify_or_arguments(List *args,
@@ -2113,47 +2112,6 @@ strip_implicit_coercions(Node *node)
    return node;
 }
 
-/*
- * set_coercionform_dontcare: set all CoercionForm fields to COERCE_DONTCARE
- *
- * This is used to make index expressions and index predicates more easily
- * comparable to clauses of queries.  CoercionForm is not semantically
- * significant (for cases where it does matter, the significant info is
- * coded into the coercion function arguments) so we can ignore it during
- * comparisons.  Thus, for example, an index on "foo::int4" can match an
- * implicit coercion to int4.
- *
- * Caution: the passed expression tree is modified in-place.
- */
-void
-set_coercionform_dontcare(Node *node)
-{
-   (void) set_coercionform_dontcare_walker(node, NULL);
-}
-
-static bool
-set_coercionform_dontcare_walker(Node *node, void *context)
-{
-   if (node == NULL)
-       return false;
-   if (IsA(node, FuncExpr))
-       ((FuncExpr *) node)->funcformat = COERCE_DONTCARE;
-   else if (IsA(node, RelabelType))
-       ((RelabelType *) node)->relabelformat = COERCE_DONTCARE;
-   else if (IsA(node, CoerceViaIO))
-       ((CoerceViaIO *) node)->coerceformat = COERCE_DONTCARE;
-   else if (IsA(node, ArrayCoerceExpr))
-       ((ArrayCoerceExpr *) node)->coerceformat = COERCE_DONTCARE;
-   else if (IsA(node, ConvertRowtypeExpr))
-       ((ConvertRowtypeExpr *) node)->convertformat = COERCE_DONTCARE;
-   else if (IsA(node, RowExpr))
-       ((RowExpr *) node)->row_format = COERCE_DONTCARE;
-   else if (IsA(node, CoerceToDomain))
-       ((CoerceToDomain *) node)->coercionformat = COERCE_DONTCARE;
-   return expression_tree_walker(node, set_coercionform_dontcare_walker,
-                                 context);
-}
-
 /*
  * Helper for eval_const_expressions: check that datatype of an attribute
  * is still what it was when the expression was parsed.  This is needed to

diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 1818a2a8718cf6fa3e32ad9397e436bcef788cbc..85315ed4bb9aebe92730d2d87d869b3547512af4 100644 (file)
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -663,12 +663,6 @@ get_relation_constraints(PlannerInfo *root,
 
            cexpr = (Node *) canonicalize_qual((Expr *) cexpr);
 
-           /*
-            * Also mark any coercion format fields as "don't care", so that
-            * we can match to both explicit and implicit coercions.
-            */
-           set_coercionform_dontcare(cexpr);
-
            /* Fix Vars to have the desired varno */
            if (varno != 1)
                ChangeVarNodes(cexpr, 1, varno, 0);

diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 4cbf812ed594b4ca291d7662fa2bd47f34f131cc..2b0b604636de5bed42bee99d38b718785deca646 100644 (file)
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3547,12 +3547,6 @@ RelationGetIndexExpressions(Relation relation)
     */
    result = (List *) eval_const_expressions(NULL, (Node *) result);
 
-   /*
-    * Also mark any coercion format fields as "don't care", so that the
-    * planner can match to both explicit and implicit coercions.
-    */
-   set_coercionform_dontcare((Node *) result);
-
    /* May as well fix opfuncids too */
    fix_opfuncids((Node *) result);
 
@@ -3619,12 +3613,6 @@ RelationGetIndexPredicate(Relation relation)
 
    result = (List *) canonicalize_qual((Expr *) result);
 
-   /*
-    * Also mark any coercion format fields as "don't care", so that the
-    * planner can match to both explicit and implicit coercions.
-    */
-   set_coercionform_dontcare((Node *) result);
-
    /* Also convert to implicit-AND format */
    result = make_ands_implicit((Expr *) result);
 

diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index cd4561dcf494e1972eed0683c05a0ca8fa2c392a..daabcb6cf9a137a670401d8901e290e5e01de621 100644 (file)
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -317,6 +317,12 @@ typedef enum CoercionContext
 
 /*
  * CoercionForm - information showing how to display a function-call node
+ *
+ * NB: equal() ignores CoercionForm fields, therefore this *must* not carry
+ * any semantically significant information.  We need that behavior so that
+ * the planner will consider equivalent implicit and explicit casts to be
+ * equivalent.  In cases where those actually behave differently, the coercion
+ * function's arguments will be different.
  */
 typedef enum CoercionForm
 {

diff --git a/src/include/optimizer/clauses.h b/src/include/optimizer/clauses.h
index ac75bd4d6ea2b0b3f11280a9ea5dc6b6d60a854b..acff7ba1b796405a739ded39fae844e8194cb678 100644 (file)
--- a/src/include/optimizer/clauses.h
+++ b/src/include/optimizer/clauses.h
@@ -79,8 +79,6 @@ extern void CommuteRowCompareExpr(RowCompareExpr *clause);
 
 extern Node *strip_implicit_coercions(Node *node);
 
-extern void set_coercionform_dontcare(Node *node);
-
 extern Node *eval_const_expressions(PlannerInfo *root, Node *node);
 
 extern Node *estimate_expression_value(PlannerInfo *root, Node *node);