Re: Bogus use of canonicalize_qual - Mailing list pgsql-hackers

I wrote:
> Whilst fooling about with predtest.c, I noticed a rather embarrassing
> error.  Consider the following, rather silly, CHECK constraint:
> ...
> So, what to do?  We have a few choices, none ideal:

I'd been assuming that we need to back-patch a fix for this, but after
further reflection, I'm not so sure.  The bug is only triggered by fairly
silly CHECK constraints, and given that it's been there a long time (at
least since 9.2 according to my tests) without any field reports, it seems
likely that nobody is writing such silly CHECK constraints.

If we suppose that we only need to fix it in HEAD, the most attractive
answer is to add a parameter distinguishing WHERE and CHECK arguments
to canonicalize_qual.  That allows symmetrical simplification of constant-
NULL subexpressions in the two cases, and the fact that the caller now
has to make an explicit choice of WHERE vs CHECK semantics might help
discourage people from applying the function in cases where it's not
clear which one applies.  PFA a patch that does it like that.

I'm a little unhappy with what I learned about the PARTITION code while
doing this :-(.  It's pretty schizophrenic about whether partition
constraints are implicit-AND or explicit-AND format, and I do not think
that the construction of default-partition constraints is done in a
desirable fashion either.  But I mostly resisted the temptation to touch
that logic in this patch.

Comments, objections?

            regards, tom lane

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index fcf7655..786c05d 100644
*** a/src/backend/catalog/partition.c
--- b/src/backend/catalog/partition.c
*************** get_proposed_default_constraint(List *ne
*** 3204,3215 ****
      defPartConstraint = makeBoolExpr(NOT_EXPR,
                                       list_make1(defPartConstraint),
                                       -1);
      defPartConstraint =
          (Expr *) eval_const_expressions(NULL,
                                          (Node *) defPartConstraint);
!     defPartConstraint = canonicalize_qual(defPartConstraint);

!     return list_make1(defPartConstraint);
  }

  /*
--- 3204,3217 ----
      defPartConstraint = makeBoolExpr(NOT_EXPR,
                                       list_make1(defPartConstraint),
                                       -1);
+
+     /* Simplify, to put the negated expression into canonical form */
      defPartConstraint =
          (Expr *) eval_const_expressions(NULL,
                                          (Node *) defPartConstraint);
!     defPartConstraint = canonicalize_qual(defPartConstraint, true);

!     return make_ands_implicit(defPartConstraint);
  }

  /*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e559afb..46a648a 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*************** PartConstraintImpliedByRelConstraint(Rel
*** 13719,13725 ****
           * fail to detect valid matches without this.
           */
          cexpr = eval_const_expressions(NULL, cexpr);
!         cexpr = (Node *) canonicalize_qual((Expr *) cexpr);

          existConstraint = list_concat(existConstraint,
                                        make_ands_implicit((Expr *) cexpr));
--- 13719,13725 ----
           * fail to detect valid matches without this.
           */
          cexpr = eval_const_expressions(NULL, cexpr);
!         cexpr = (Node *) canonicalize_qual((Expr *) cexpr, true);

          existConstraint = list_concat(existConstraint,
                                        make_ands_implicit((Expr *) cexpr));
*************** ATExecAttachPartition(List **wqueue, Rel
*** 14058,14067 ****
      /* Skip validation if there are no constraints to validate. */
      if (partConstraint)
      {
          partConstraint =
              (List *) eval_const_expressions(NULL,
                                              (Node *) partConstraint);
!         partConstraint = (List *) canonicalize_qual((Expr *) partConstraint);
          partConstraint = list_make1(make_ands_explicit(partConstraint));

          /*
--- 14058,14075 ----
      /* Skip validation if there are no constraints to validate. */
      if (partConstraint)
      {
+         /*
+          * Run the partition quals through const-simplification similar to
+          * check constraints.  We skip canonicalize_qual, though, because
+          * partition quals should be in canonical form already; also, since
+          * the qual is in implicit-AND format, we'd have to explicitly convert
+          * it to explicit-AND format and back again.
+          */
          partConstraint =
              (List *) eval_const_expressions(NULL,
                                              (Node *) partConstraint);
!
!         /* XXX this sure looks wrong */
          partConstraint = list_make1(make_ands_explicit(partConstraint));

          /*
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 14b7bec..24e6c46 100644
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** preprocess_expression(PlannerInfo *root,
*** 988,994 ****
       */
      if (kind == EXPRKIND_QUAL)
      {
!         expr = (Node *) canonicalize_qual((Expr *) expr);

  #ifdef OPTIMIZER_DEBUG
          printf("After canonicalize_qual()\n");
--- 988,994 ----
       */
      if (kind == EXPRKIND_QUAL)
      {
!         expr = (Node *) canonicalize_qual((Expr *) expr, false);

  #ifdef OPTIMIZER_DEBUG
          printf("After canonicalize_qual()\n");
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 46367cb..dc86dd5 100644
*** a/src/backend/optimizer/plan/subselect.c
--- b/src/backend/optimizer/plan/subselect.c
*************** convert_EXISTS_to_ANY(PlannerInfo *root,
*** 1740,1746 ****
       * subroot.
       */
      whereClause = eval_const_expressions(root, whereClause);
!     whereClause = (Node *) canonicalize_qual((Expr *) whereClause);
      whereClause = (Node *) make_ands_implicit((Expr *) whereClause);

      /*
--- 1740,1746 ----
       * subroot.
       */
      whereClause = eval_const_expressions(root, whereClause);
!     whereClause = (Node *) canonicalize_qual((Expr *) whereClause, false);
      whereClause = (Node *) make_ands_implicit((Expr *) whereClause);

      /*
diff --git a/src/backend/optimizer/prep/prepqual.c b/src/backend/optimizer/prep/prepqual.c
index cb1f485..52f8893 100644
*** a/src/backend/optimizer/prep/prepqual.c
--- b/src/backend/optimizer/prep/prepqual.c
***************
*** 39,45 ****

  static List *pull_ands(List *andlist);
  static List *pull_ors(List *orlist);
! static Expr *find_duplicate_ors(Expr *qual);
  static Expr *process_duplicate_ors(List *orlist);


--- 39,45 ----

  static List *pull_ands(List *andlist);
  static List *pull_ors(List *orlist);
! static Expr *find_duplicate_ors(Expr *qual, bool is_check);
  static Expr *process_duplicate_ors(List *orlist);


*************** negate_clause(Node *node)
*** 269,274 ****
--- 269,279 ----
   * canonicalize_qual
   *      Convert a qualification expression to the most useful form.
   *
+  * This is primarily intended to be used on top-level WHERE (or JOIN/ON)
+  * clauses.  It can also be used on top-level CHECK constraints, for which
+  * pass is_check = true.  DO NOT call it on any expression that is not known
+  * to be one or the other, as it might apply inappropriate simplifications.
+  *
   * The name of this routine is a holdover from a time when it would try to
   * force the expression into canonical AND-of-ORs or OR-of-ANDs form.
   * Eventually, we recognized that that had more theoretical purity than
*************** negate_clause(Node *node)
*** 283,289 ****
   * Returns the modified qualification.
   */
  Expr *
! canonicalize_qual(Expr *qual)
  {
      Expr       *newqual;

--- 288,294 ----
   * Returns the modified qualification.
   */
  Expr *
! canonicalize_qual(Expr *qual, bool is_check)
  {
      Expr       *newqual;

*************** canonicalize_qual(Expr *qual)
*** 291,302 ****
      if (qual == NULL)
          return NULL;

      /*
       * Pull up redundant subclauses in OR-of-AND trees.  We do this only
       * within the top-level AND/OR structure; there's no point in looking
       * deeper.  Also remove any NULL constants in the top-level structure.
       */
!     newqual = find_duplicate_ors(qual);

      return newqual;
  }
--- 296,310 ----
      if (qual == NULL)
          return NULL;

+     /* This should not be invoked on quals in implicit-AND format */
+     Assert(!IsA(qual, List));
+
      /*
       * Pull up redundant subclauses in OR-of-AND trees.  We do this only
       * within the top-level AND/OR structure; there's no point in looking
       * deeper.  Also remove any NULL constants in the top-level structure.
       */
!     newqual = find_duplicate_ors(qual, is_check);

      return newqual;
  }
*************** pull_ors(List *orlist)
*** 395,410 ****
   *      Only the top-level AND/OR structure is searched.
   *
   * While at it, we remove any NULL constants within the top-level AND/OR
!  * structure, eg "x OR NULL::boolean" is reduced to "x".  In general that
!  * would change the result, so eval_const_expressions can't do it; but at
!  * top level of WHERE, we don't need to distinguish between FALSE and NULL
!  * results, so it's valid to treat NULL::boolean the same as FALSE and then
!  * simplify AND/OR accordingly.
   *
   * Returns the modified qualification.  AND/OR flatness is preserved.
   */
  static Expr *
! find_duplicate_ors(Expr *qual)
  {
      if (or_clause((Node *) qual))
      {
--- 403,419 ----
   *      Only the top-level AND/OR structure is searched.
   *
   * While at it, we remove any NULL constants within the top-level AND/OR
!  * structure, eg in a WHERE clause, "x OR NULL::boolean" is reduced to "x".
!  * In general that would change the result, so eval_const_expressions can't
!  * do it; but at top level of WHERE, we don't need to distinguish between
!  * FALSE and NULL results, so it's valid to treat NULL::boolean the same
!  * as FALSE and then simplify AND/OR accordingly.  Conversely, in a top-level
!  * CHECK constraint, we may treat a NULL the same as TRUE.
   *
   * Returns the modified qualification.  AND/OR flatness is preserved.
   */
  static Expr *
! find_duplicate_ors(Expr *qual, bool is_check)
  {
      if (or_clause((Node *) qual))
      {
*************** find_duplicate_ors(Expr *qual)
*** 416,433 ****
          {
              Expr       *arg = (Expr *) lfirst(temp);

!             arg = find_duplicate_ors(arg);

              /* Get rid of any constant inputs */
              if (arg && IsA(arg, Const))
              {
                  Const       *carg = (Const *) arg;

!                 /* Drop constant FALSE or NULL */
!                 if (carg->constisnull || !DatumGetBool(carg->constvalue))
!                     continue;
!                 /* constant TRUE, so OR reduces to TRUE */
!                 return arg;
              }

              orlist = lappend(orlist, arg);
--- 425,453 ----
          {
              Expr       *arg = (Expr *) lfirst(temp);

!             arg = find_duplicate_ors(arg, is_check);

              /* Get rid of any constant inputs */
              if (arg && IsA(arg, Const))
              {
                  Const       *carg = (Const *) arg;

!                 if (is_check)
!                 {
!                     /* Within OR in CHECK, drop constant FALSE */
!                     if (!carg->constisnull && !DatumGetBool(carg->constvalue))
!                         continue;
!                     /* Constant TRUE or NULL, so OR reduces to TRUE */
!                     return (Expr *) makeBoolConst(true, false);
!                 }
!                 else
!                 {
!                     /* Within OR in WHERE, drop constant FALSE or NULL */
!                     if (carg->constisnull || !DatumGetBool(carg->constvalue))
!                         continue;
!                     /* Constant TRUE, so OR reduces to TRUE */
!                     return arg;
!                 }
              }

              orlist = lappend(orlist, arg);
*************** find_duplicate_ors(Expr *qual)
*** 449,466 ****
          {
              Expr       *arg = (Expr *) lfirst(temp);

!             arg = find_duplicate_ors(arg);

              /* Get rid of any constant inputs */
              if (arg && IsA(arg, Const))
              {
                  Const       *carg = (Const *) arg;

!                 /* Drop constant TRUE */
!                 if (!carg->constisnull && DatumGetBool(carg->constvalue))
!                     continue;
!                 /* constant FALSE or NULL, so AND reduces to FALSE */
!                 return (Expr *) makeBoolConst(false, false);
              }

              andlist = lappend(andlist, arg);
--- 469,497 ----
          {
              Expr       *arg = (Expr *) lfirst(temp);

!             arg = find_duplicate_ors(arg, is_check);

              /* Get rid of any constant inputs */
              if (arg && IsA(arg, Const))
              {
                  Const       *carg = (Const *) arg;

!                 if (is_check)
!                 {
!                     /* Within AND in CHECK, drop constant TRUE or NULL */
!                     if (carg->constisnull || DatumGetBool(carg->constvalue))
!                         continue;
!                     /* Constant FALSE, so AND reduces to FALSE */
!                     return arg;
!                 }
!                 else
!                 {
!                     /* Within AND in WHERE, drop constant TRUE */
!                     if (!carg->constisnull && DatumGetBool(carg->constvalue))
!                         continue;
!                     /* Constant FALSE or NULL, so AND reduces to FALSE */
!                     return (Expr *) makeBoolConst(false, false);
!                 }
              }

              andlist = lappend(andlist, arg);
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 7c4cd8a..bd3a0c4 100644
*** a/src/backend/optimizer/util/plancat.c
--- b/src/backend/optimizer/util/plancat.c
*************** get_relation_constraints(PlannerInfo *ro
*** 1209,1215 ****
               */
              cexpr = eval_const_expressions(root, cexpr);

!             cexpr = (Node *) canonicalize_qual((Expr *) cexpr);

              /* Fix Vars to have the desired varno */
              if (varno != 1)
--- 1209,1215 ----
               */
              cexpr = eval_const_expressions(root, cexpr);

!             cexpr = (Node *) canonicalize_qual((Expr *) cexpr, true);

              /* Fix Vars to have the desired varno */
              if (varno != 1)
*************** get_relation_constraints(PlannerInfo *ro
*** 1262,1272 ****
      if (pcqual)
      {
          /*
!          * Run each expression through const-simplification and
!          * canonicalization similar to check constraints.
           */
          pcqual = (List *) eval_const_expressions(root, (Node *) pcqual);
-         pcqual = (List *) canonicalize_qual((Expr *) pcqual);

          /* Fix Vars to have the desired varno */
          if (varno != 1)
--- 1262,1274 ----
      if (pcqual)
      {
          /*
!          * Run the partition quals through const-simplification similar to
!          * check constraints.  We skip canonicalize_qual, though, because
!          * partition quals should be in canonical form already; also, since
!          * the qual is in implicit-AND format, we'd have to explicitly convert
!          * it to explicit-AND format and back again.
           */
          pcqual = (List *) eval_const_expressions(root, (Node *) pcqual);

          /* Fix Vars to have the desired varno */
          if (varno != 1)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9ee78f8..6ab4db2 100644
*** a/src/backend/utils/cache/relcache.c
--- b/src/backend/utils/cache/relcache.c
*************** RelationBuildPartitionKey(Relation relat
*** 900,907 ****
           * will be comparing them to similarly-processed qual clause operands,
           * and may fail to detect valid matches without this step; fix
           * opfuncids while at it.  We don't need to bother with
!          * canonicalize_qual() though, because partition expressions are not
!          * full-fledged qualification clauses.
           */
          expr = eval_const_expressions(NULL, expr);
          fix_opfuncids(expr);
--- 900,908 ----
           * will be comparing them to similarly-processed qual clause operands,
           * and may fail to detect valid matches without this step; fix
           * opfuncids while at it.  We don't need to bother with
!          * canonicalize_qual() though, because partition expressions should be
!          * in canonical form already (ie, no need for OR-merging or constant
!          * elimination).
           */
          expr = eval_const_expressions(NULL, expr);
          fix_opfuncids(expr);
*************** RelationGetIndexExpressions(Relation rel
*** 4713,4724 ****
       * Run the expressions through eval_const_expressions. This is not just an
       * optimization, but is necessary, because the planner will be comparing
       * them to similarly-processed qual clauses, and may fail to detect valid
!      * matches without this.  We don't bother with canonicalize_qual, however.
       */
      result = (List *) eval_const_expressions(NULL, (Node *) result);

-     result = (List *) canonicalize_qual((Expr *) result);
-
      /* May as well fix opfuncids too */
      fix_opfuncids((Node *) result);

--- 4714,4724 ----
       * Run the expressions through eval_const_expressions. This is not just an
       * optimization, but is necessary, because the planner will be comparing
       * them to similarly-processed qual clauses, and may fail to detect valid
!      * matches without this.  We must not use canonicalize_qual, however,
!      * since these aren't qual expressions.
       */
      result = (List *) eval_const_expressions(NULL, (Node *) result);

      /* May as well fix opfuncids too */
      fix_opfuncids((Node *) result);

*************** RelationGetIndexPredicate(Relation relat
*** 4783,4789 ****
       */
      result = (List *) eval_const_expressions(NULL, (Node *) result);

!     result = (List *) canonicalize_qual((Expr *) result);

      /* Also convert to implicit-AND format */
      result = make_ands_implicit((Expr *) result);
--- 4783,4789 ----
       */
      result = (List *) eval_const_expressions(NULL, (Node *) result);

!     result = (List *) canonicalize_qual((Expr *) result, false);

      /* Also convert to implicit-AND format */
      result = make_ands_implicit((Expr *) result);
diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h
index 89b7ef3..3860877 100644
*** a/src/include/optimizer/prep.h
--- b/src/include/optimizer/prep.h
*************** extern Relids get_relids_for_join(Planne
*** 33,39 ****
   * prototypes for prepqual.c
   */
  extern Node *negate_clause(Node *node);
! extern Expr *canonicalize_qual(Expr *qual);

  /*
   * prototypes for preptlist.c
--- 33,39 ----
   * prototypes for prepqual.c
   */
  extern Node *negate_clause(Node *node);
! extern Expr *canonicalize_qual(Expr *qual, bool is_check);

  /*
   * prototypes for preptlist.c
diff --git a/src/test/modules/test_predtest/test_predtest.c b/src/test/modules/test_predtest/test_predtest.c
index 4a3b14a..51320ad 100644
*** a/src/test/modules/test_predtest/test_predtest.c
--- b/src/test/modules/test_predtest/test_predtest.c
***************
*** 19,25 ****
  #include "funcapi.h"
  #include "optimizer/clauses.h"
  #include "optimizer/predtest.h"
- #include "optimizer/prep.h"
  #include "utils/builtins.h"

  PG_MODULE_MAGIC;
--- 19,24 ----
*************** test_predtest(PG_FUNCTION_ARGS)
*** 137,154 ****

      /*
       * Because the clauses are in the SELECT list, preprocess_expression did
!      * not pass them through canonicalize_qual nor make_ands_implicit.  We can
!      * do that here, though, and should do so to match the planner's normal
!      * usage of the predicate proof functions.
       *
!      * This still does not exactly duplicate the normal usage of the proof
!      * functions, in that they are often given qual clauses containing
!      * RestrictInfo nodes.  But since predtest.c just looks through those
!      * anyway, it seems OK to not worry about that point.
       */
-     clause1 = canonicalize_qual(clause1);
-     clause2 = canonicalize_qual(clause2);
-
      clause1 = (Expr *) make_ands_implicit(clause1);
      clause2 = (Expr *) make_ands_implicit(clause2);

--- 136,153 ----

      /*
       * Because the clauses are in the SELECT list, preprocess_expression did
!      * not pass them through canonicalize_qual nor make_ands_implicit.
       *
!      * We can't do canonicalize_qual here, since it's unclear whether the
!      * expressions ought to be treated as WHERE or CHECK clauses. Fortunately,
!      * useful test expressions wouldn't be affected by those transformations
!      * anyway.  We should do make_ands_implicit, though.
!      *
!      * Another way in which this does not exactly duplicate the normal usage
!      * of the proof functions is that they are often given qual clauses
!      * containing RestrictInfo nodes.  But since predtest.c just looks through
!      * those anyway, it seems OK to not worry about that point.
       */
      clause1 = (Expr *) make_ands_implicit(clause1);
      clause2 = (Expr *) make_ands_implicit(clause2);

diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index a79f891..d768dc0 100644
*** a/src/test/regress/expected/inherit.out
--- b/src/test/regress/expected/inherit.out
*************** reset enable_seqscan;
*** 1661,1666 ****
--- 1661,1690 ----
  reset enable_indexscan;
  reset enable_bitmapscan;
  --
+ -- Check handling of a constant-null CHECK constraint
+ --
+ create table cnullparent (f1 int);
+ create table cnullchild (check (f1 = 1 or f1 = null)) inherits(cnullparent);
+ insert into cnullchild values(1);
+ insert into cnullchild values(2);
+ insert into cnullchild values(null);
+ select * from cnullparent;
+  f1
+ ----
+   1
+   2
+
+ (3 rows)
+
+ select * from cnullparent where f1 = 2;
+  f1
+ ----
+   2
+ (1 row)
+
+ drop table cnullparent cascade;
+ NOTICE:  drop cascades to table cnullchild
+ --
  -- Check that constraint exclusion works correctly with partitions using
  -- implicit constraints generated from the partition bound information.
  --
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 2e42ae1..9397f72 100644
*** a/src/test/regress/sql/inherit.sql
--- b/src/test/regress/sql/inherit.sql
*************** reset enable_indexscan;
*** 612,617 ****
--- 612,629 ----
  reset enable_bitmapscan;

  --
+ -- Check handling of a constant-null CHECK constraint
+ --
+ create table cnullparent (f1 int);
+ create table cnullchild (check (f1 = 1 or f1 = null)) inherits(cnullparent);
+ insert into cnullchild values(1);
+ insert into cnullchild values(2);
+ insert into cnullchild values(null);
+ select * from cnullparent;
+ select * from cnullparent where f1 = 2;
+ drop table cnullparent cascade;
+
+ --
  -- Check that constraint exclusion works correctly with partitions using
  -- implicit constraints generated from the partition bound information.
  --