fix-canonicalization-of-check-constraints-1.patch

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.
  --