Re: Making the planner more tolerant of implicit/explicit casts - Mailing list pgsql-hackers

I wrote:
> What I'm thinking about is modifying eval_const_expressions so that
> one of its responsibilities is to force CoercionForm fields to
> COERCE_DONTCARE in the output;

I fooled around with that approach for awhile and didn't like the
results, mainly because it caused EXPLAIN output to change in unpleasant
ways (ruleutils.c needs the CoercionForm info to format its output nicely).

However, on further reflection I realized that we could fix it just by
making equal() ignore CoercionForm fields altogether.  I remember having
considered that and shied away from it back when I first invented the
COERCE_DONTCARE hack, on the grounds that it would put too much semantic
awareness into equal().  However, we've long since abandoned the idea
that equal() should insist on full bitwise equality of nodes --- it's
ignored location fields for some time without ill effects, and there are
a number of other special cases in there too.  So as long as we're
willing to consider that equal() can mean just semantic equivalence of
two node trees, this can be fixed by removing code rather than adding
it, along the lines of the attached patch.

We could take the further step of removing the COERCE_DONTCARE enum
value altogether; the remaining uses are only to fill in CoercionForm
fields in nodes that the planner creates out of whole cloth, and now we
could make it fill in one of the more standard values instead.  I didn't
do that in the attached because it makes the patch longer but no more
enlightening (and in any case I don't think that change would be good to
back-patch).

I'm reasonably convinced that this is a good fix for HEAD, but am of two
minds whether to back-patch it or not.  The problem complained of in
bug #7598 may seem a bit narrow, but the real point is that whether you
write a cast explicitly or not shouldn't affect planning if the
semantics are the same.  This might well be a significant though
previously unrecognized performance issue, particularly for people who
use varchar columns heavily.

Thoughts?

            regards, tom lane

diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 226b99a1d271d425fdc394dd3d3016661cecabf5..95a95f477bead7aee3b484c01f3802a6b81efc3b 100644
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 83,88 ****
--- 83,92 ----
  #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
*************** _equalFuncExpr(const FuncExpr *a, const
*** 235,250 ****
      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_SCALAR_FIELD(funccollid);
      COMPARE_SCALAR_FIELD(inputcollid);
      COMPARE_NODE_FIELD(args);
--- 239,245 ----
      COMPARE_SCALAR_FIELD(funcid);
      COMPARE_SCALAR_FIELD(funcresulttype);
      COMPARE_SCALAR_FIELD(funcretset);
!     COMPARE_COERCIONFORM_FIELD(funcformat);
      COMPARE_SCALAR_FIELD(funccollid);
      COMPARE_SCALAR_FIELD(inputcollid);
      COMPARE_NODE_FIELD(args);
*************** _equalRelabelType(const RelabelType *a,
*** 448,463 ****
      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_LOCATION_FIELD(location);

      return true;
--- 443,449 ----
      COMPARE_SCALAR_FIELD(resulttype);
      COMPARE_SCALAR_FIELD(resulttypmod);
      COMPARE_SCALAR_FIELD(resultcollid);
!     COMPARE_COERCIONFORM_FIELD(relabelformat);
      COMPARE_LOCATION_FIELD(location);

      return true;
*************** _equalCoerceViaIO(const CoerceViaIO *a,
*** 469,484 ****
      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_LOCATION_FIELD(location);

      return true;
--- 455,461 ----
      COMPARE_NODE_FIELD(arg);
      COMPARE_SCALAR_FIELD(resulttype);
      COMPARE_SCALAR_FIELD(resultcollid);
!     COMPARE_COERCIONFORM_FIELD(coerceformat);
      COMPARE_LOCATION_FIELD(location);

      return true;
*************** _equalArrayCoerceExpr(const ArrayCoerceE
*** 493,508 ****
      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_LOCATION_FIELD(location);

      return true;
--- 470,476 ----
      COMPARE_SCALAR_FIELD(resulttypmod);
      COMPARE_SCALAR_FIELD(resultcollid);
      COMPARE_SCALAR_FIELD(isExplicit);
!     COMPARE_COERCIONFORM_FIELD(coerceformat);
      COMPARE_LOCATION_FIELD(location);

      return true;
*************** _equalConvertRowtypeExpr(const ConvertRo
*** 513,528 ****
  {
      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_LOCATION_FIELD(location);

      return true;
--- 481,487 ----
  {
      COMPARE_NODE_FIELD(arg);
      COMPARE_SCALAR_FIELD(resulttype);
!     COMPARE_COERCIONFORM_FIELD(convertformat);
      COMPARE_LOCATION_FIELD(location);

      return true;
*************** _equalRowExpr(const RowExpr *a, const Ro
*** 589,604 ****
  {
      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_NODE_FIELD(colnames);
      COMPARE_LOCATION_FIELD(location);

--- 548,554 ----
  {
      COMPARE_NODE_FIELD(args);
      COMPARE_SCALAR_FIELD(row_typeid);
!     COMPARE_COERCIONFORM_FIELD(row_format);
      COMPARE_NODE_FIELD(colnames);
      COMPARE_LOCATION_FIELD(location);

*************** _equalCoerceToDomain(const CoerceToDomai
*** 684,699 ****
      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_LOCATION_FIELD(location);

      return true;
--- 634,640 ----
      COMPARE_SCALAR_FIELD(resulttype);
      COMPARE_SCALAR_FIELD(resulttypmod);
      COMPARE_SCALAR_FIELD(resultcollid);
!     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 fa6817c9148439116e211aec0582f4a190654faa..5894dd39c178150948f429cabf31396f12c634ee 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** static bool contain_leaky_functions_walk
*** 98,104 ****
  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,
--- 98,103 ----
*************** strip_implicit_coercions(Node *node)
*** 2115,2161 ****
  }

  /*
-  * 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
   * guard against improper simplification after ALTER COLUMN TYPE.  (XXX we
--- 2114,2119 ----
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 25f8785b6395cf913357774cd733b39aa65e3b3e..abcd0ee574cf4f7560e71d8fdc11ebea5a9b303c 100644
*** a/src/backend/optimizer/util/plancat.c
--- b/src/backend/optimizer/util/plancat.c
*************** get_relation_constraints(PlannerInfo *ro
*** 665,676 ****

              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);
--- 665,670 ----
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 0cdd30d15b1167da4b79984d0412b9b5797e8a9b..8c9ebe0f6f89cda440fae64299e6ba5d59aff150 100644
*** a/src/backend/utils/cache/relcache.c
--- b/src/backend/utils/cache/relcache.c
*************** RelationGetIndexExpressions(Relation rel
*** 3549,3560 ****
       */
      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);

--- 3549,3554 ----
*************** RelationGetIndexPredicate(Relation relat
*** 3621,3632 ****

      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);

--- 3615,3620 ----
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index cd4561dcf494e1972eed0683c05a0ca8fa2c392a..daabcb6cf9a137a670401d8901e290e5e01de621 100644
*** a/src/include/nodes/primnodes.h
--- b/src/include/nodes/primnodes.h
*************** typedef enum CoercionContext
*** 317,322 ****
--- 317,328 ----

  /*
   * 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
*** a/src/include/optimizer/clauses.h
--- b/src/include/optimizer/clauses.h
*************** extern void CommuteRowCompareExpr(RowCom
*** 79,86 ****

  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);
--- 79,84 ----