Fix improper uses of canonicalize_qual().

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 d007a9505 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 f0e44751d 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/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 716af49d465491580ceb470d688f82f89a33fe63..615f1cbc01dbc5f28c0473c5e121fc9d9ebf9403 100644 (file)
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -742,7 +742,7 @@ preprocess_expression(PlannerInfo *root, Node *expr, int kind)
         */
        if (kind == EXPRKIND_QUAL)
        {
-               expr = (Node *) canonicalize_qual((Expr *) expr);
+               expr = (Node *) canonicalize_qual_ext((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 e400ea983cce33a07ad6dc833be990775322f05e..9c7172c775146d8bf5485f32d13fcc86de8efd96 100644 (file)
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -1696,7 +1696,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_ext((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 1176e819dd0744a7373c2d2201a1b41388f610dc..7e8b8e7abcab22ffce1cc17b966e1a1032bc135a 100644 (file)
--- 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,24 @@ negate_clause(Node *node)
  * canonicalize_qual
  *       Convert a qualification expression to the most useful form.
  *
+ * Backwards-compatibility wrapper for use by external code that hasn't
+ * been updated.
+ */
+Expr *
+canonicalize_qual(Expr *qual)
+{
+       return canonicalize_qual_ext(qual, false);
+}
+
+/*
+ * canonicalize_qual_ext
+ *       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,7 +301,7 @@ negate_clause(Node *node)
  * Returns the modified qualification.
  */
 Expr *
-canonicalize_qual(Expr *qual)
+canonicalize_qual_ext(Expr *qual, bool is_check)
 {
        Expr       *newqual;
 
@@ -296,7 +314,7 @@ canonicalize_qual(Expr *qual)
         * 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 +413,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 +435,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 +479,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
index f037f90d9852e6ac14600b7ee9ac87a456d06a5a..ac57d0f2b89899db210c737b69ea6da4a55e2d13 100644 (file)
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -1056,7 +1056,7 @@ get_relation_constraints(PlannerInfo *root,
                         */
                        cexpr = eval_const_expressions(root, cexpr);
 
-                       cexpr = (Node *) canonicalize_qual((Expr *) cexpr);
+                       cexpr = (Node *) canonicalize_qual_ext((Expr *) cexpr, true);
 
                        /* 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 4ebe103c1ad1f57e0695c8761482679fe0bc7f18..afaf8794f00c8c16dd7cf0b5a792dd0183e6f619 100644 (file)
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -4091,7 +4091,8 @@ 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);
 
@@ -4159,7 +4160,7 @@ RelationGetIndexPredicate(Relation relation)
         */
        result = (List *) eval_const_expressions(NULL, (Node *) result);
 
-       result = (List *) canonicalize_qual((Expr *) result);
+       result = (List *) canonicalize_qual_ext((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 46fadff5360d6e1342c8c99d308097735fc4138c..f87bcb6e855617c433e8596d2a1795d06bd0f551 100644 (file)
--- a/src/include/optimizer/prep.h
+++ b/src/include/optimizer/prep.h
@@ -34,6 +34,7 @@ extern Relids get_relids_for_join(PlannerInfo *root, int joinrelid);
  */
 extern Node *negate_clause(Node *node);
 extern Expr *canonicalize_qual(Expr *qual);
+extern Expr *canonicalize_qual_ext(Expr *qual, bool is_check);
 
 /*
  * prototypes for prepsecurity.c

diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 4f465a5f1efe4a229501ef82ce1f019c06987571..50d3403011adf74ed58c870fa802e2b13fcdf8b1 100644 (file)
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1591,3 +1591,27 @@ FROM generate_series(1, 3) g(i);
 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

diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 0e390cda3e4d31899bb4c30fe146da81d7e0b569..385359b76112581b59f0fcb18047ccd11b2a3370 100644 (file)
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -562,3 +562,15 @@ FROM generate_series(1, 3) g(i);
 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;