Fix improper uses of canonicalize_qual().

tglsfdc · tglsfdc · commit 4a4e2442a7f7 · 2018-03-11T18:10:42.000-04:00
One of the things canonicalize_qual() does is to remove constant-NULL subexpressions of top-level AND/OR clauses. It does that on the assumption that what it's given is a top-level WHERE clause, so that NULL can be treated like FALSE. Although this is documented down inside a subroutine of canonicalize_qual(), it wasn't mentioned in the documentation of that function itself, and some callers hadn't gotten that memo. Notably, commit d007a95 caused get_relation_constraints() to apply canonicalize_qual() to CHECK constraints. That allowed constraint exclusion to misoptimize situations in which a CHECK constraint had a provably-NULL subclause, as seen in the regression test case added here, in which a child table that should be scanned is not. (Although this thinko is ancient, the test case doesn't fail before 9.2, for reasons I've not bothered to track down in detail. There may be related cases that do fail before that.) More recently, commit f0e4475 added an independent bug by applying canonicalize_qual() to index expressions, which is even sillier since those might not even be boolean. If they are, though, I think this could lead to making incorrect index entries for affected index expressions in v10. I haven't attempted to prove that though. To fix, add an "is_check" parameter to canonicalize_qual() to specify whether it should assume WHERE or CHECK semantics, and make it perform NULL-elimination accordingly. Adjust the callers to apply the right semantics, or remove the call entirely in cases where it's not known that the expression has one or the other semantics. I also removed the call in some cases involving partition expressions, where it should be a no-op because such expressions should be canonical already ... and was a no-op, independently of whether it could in principle have done something, because it was being handed the qual in implicit-AND format which isn't what it expects. In HEAD, add an Assert to catch that type of mistake in future. This represents an API break for external callers of canonicalize_qual(). While that's intentional in HEAD to make such callers think about which case applies to them, it seems like something we probably wouldn't be thanked for in released branches. Hence, in released branches, the extra parameter is added to a new function canonicalize_qual_ext(), and canonicalize_qual() is a wrapper that retains its old behavior. Patch by me with suggestions from Dean Rasheed. Back-patch to all supported branches. Discussion: https://postgr.es/m/24475.1520635069@sss.pgh.pa.us
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
@@ -3204,12 +3204,14 @@ get_proposed_default_constraint(List *new_part_constraints)
 	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);
+	defPartConstraint = canonicalize_qual(defPartConstraint, true);
 
-	return list_make1(defPartConstraint);
+	return make_ands_implicit(defPartConstraint);
 }
 
 /*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
@@ -13719,7 +13719,7 @@ PartConstraintImpliedByRelConstraint(Relation scanrel,
 		 * fail to detect valid matches without this.
 		 */
 		cexpr = eval_const_expressions(NULL, cexpr);
-		cexpr = (Node *) canonicalize_qual((Expr *) cexpr);
+		cexpr = (Node *) canonicalize_qual((Expr *) cexpr, true);
 
 		existConstraint = list_concat(existConstraint,
 									  make_ands_implicit((Expr *) cexpr));
@@ -14058,10 +14058,18 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd)
 	/* 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);
-		partConstraint = (List *) canonicalize_qual((Expr *) 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
@@ -988,7 +988,7 @@ preprocess_expression(PlannerInfo *root, Node *expr, int kind)
 	 */
 	if (kind == EXPRKIND_QUAL)
 	{
-		expr = (Node *) canonicalize_qual((Expr *) expr);
+		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
@@ -1740,7 +1740,7 @@ convert_EXISTS_to_ANY(PlannerInfo *root, Query *subselect,
 	 * subroot.
 	 */
 	whereClause = eval_const_expressions(root, whereClause);
-	whereClause = (Node *) canonicalize_qual((Expr *) 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
@@ -39,7 +39,7 @@
 
 static List *pull_ands(List *andlist);
 static List *pull_ors(List *orlist);
-static Expr *find_duplicate_ors(Expr *qual);
+static Expr *find_duplicate_ors(Expr *qual, bool is_check);
 static Expr *process_duplicate_ors(List *orlist);
 
 
@@ -269,6 +269,11 @@ negate_clause(Node *node)
  * 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
@@ -283,20 +288,23 @@ negate_clause(Node *node)
  * Returns the modified qualification.
  */
 Expr *
-canonicalize_qual(Expr *qual)
+canonicalize_qual(Expr *qual, bool is_check)
 {
 	Expr	   *newqual;
 
 	/* Quick exit for empty qual */
 	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);
+	newqual = find_duplicate_ors(qual, is_check);
 
 	return newqual;
 }
@@ -395,16 +403,17 @@ pull_ors(List *orlist)
  *	  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.
+ * 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)
+find_duplicate_ors(Expr *qual, bool is_check)
 {
 	if (or_clause((Node *) qual))
 	{
@@ -416,18 +425,29 @@ find_duplicate_ors(Expr *qual)
 		{
 			Expr	   *arg = (Expr *) lfirst(temp);
 
-			arg = find_duplicate_ors(arg);
+			arg = find_duplicate_ors(arg, is_check);
 
 			/* 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;
+				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);
@@ -449,18 +469,29 @@ find_duplicate_ors(Expr *qual)
 		{
 			Expr	   *arg = (Expr *) lfirst(temp);
 
-			arg = find_duplicate_ors(arg);
+			arg = find_duplicate_ors(arg, is_check);
 
 			/* 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);
+				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
@@ -1209,7 +1209,7 @@ get_relation_constraints(PlannerInfo *root,
 			 */
 			cexpr = eval_const_expressions(root, cexpr);
 
-			cexpr = (Node *) canonicalize_qual((Expr *) cexpr);
+			cexpr = (Node *) canonicalize_qual((Expr *) cexpr, true);
 
 			/* Fix Vars to have the desired varno */
 			if (varno != 1)
@@ -1262,11 +1262,13 @@ get_relation_constraints(PlannerInfo *root,
 	if (pcqual)
 	{
 		/*
-		 * Run each expression through const-simplification and
-		 * canonicalization similar to check constraints.
+		 * 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);
-		pcqual = (List *) canonicalize_qual((Expr *) 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
@@ -900,8 +900,9 @@ RelationBuildPartitionKey(Relation relation)
 		 * 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.
+		 * 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);
@@ -4713,12 +4714,11 @@ RelationGetIndexExpressions(Relation relation)
 	 * 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.
+	 * matches without this.  We must not use canonicalize_qual, however,
+	 * since these aren't qual expressions.
 	 */
 	result = (List *) eval_const_expressions(NULL, (Node *) result);
 
-	result = (List *) canonicalize_qual((Expr *) result);
-
 	/* May as well fix opfuncids too */
 	fix_opfuncids((Node *) result);
 
@@ -4783,7 +4783,7 @@ RelationGetIndexPredicate(Relation relation)
 	 */
 	result = (List *) eval_const_expressions(NULL, (Node *) result);
 
-	result = (List *) canonicalize_qual((Expr *) 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
@@ -33,7 +33,7 @@ extern Relids get_relids_for_join(PlannerInfo *root, int joinrelid);
  * prototypes for prepqual.c
  */
 extern Node *negate_clause(Node *node);
-extern Expr *canonicalize_qual(Expr *qual);
+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
@@ -19,7 +19,6 @@
 #include "funcapi.h"
 #include "optimizer/clauses.h"
 #include "optimizer/predtest.h"
-#include "optimizer/prep.h"
 #include "utils/builtins.h"
 
 PG_MODULE_MAGIC;
@@ -137,18 +136,18 @@ test_predtest(PG_FUNCTION_ARGS)
 
 	/*
 	 * 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.
+	 * not pass them through canonicalize_qual nor make_ands_implicit.
 	 *
-	 * 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.
+	 * 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 = canonicalize_qual(clause1);
-	clause2 = canonicalize_qual(clause2);
-
 	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
@@ -1661,6 +1661,30 @@ reset enable_seqscan;
 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
@@ -611,6 +611,18 @@ reset enable_seqscan;
 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;
+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.

Original file line number	Diff line number	Diff line change
`@@ -988,7 +988,7 @@ preprocess_expression(PlannerInfo root, Node expr, int kind)`
`988`	`988`	`*/`
`989`	`989`	`if (kind == EXPRKIND_QUAL)`
`990`	`990`	`{`
`991`		`- expr = (Node ) canonicalize_qual((Expr ) expr);`
	`991`	`+ expr = (Node ) canonicalize_qual((Expr ) expr, false);`
`992`	`992`
`993`	`993`	`#ifdef OPTIMIZER_DEBUG`
`994`	`994`	`printf("After canonicalize_qual()\n");`