diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index fcf7655553..786c05df73 100644 --- 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 index e559afb9df..46a648a33a 100644 --- 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 index 14b7becf3e..24e6c46396 100644 --- 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 index 46367cba63..dc86dd5a0b 100644 --- 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 index cb1f4853c3..52f8893f4f 100644 --- 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,7 +288,7 @@ negate_clause(Node *node) * Returns the modified qualification. */ Expr * -canonicalize_qual(Expr *qual) +canonicalize_qual(Expr *qual, bool is_check) { Expr *newqual; @@ -291,12 +296,15 @@ canonicalize_qual(Expr *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 index 7c4cd8a7f9..bd3a0c4a0a 100644 --- 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 index 9ee78f885f..6ab4db26bd 100644 --- 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 index 89b7ef337c..38608770a2 100644 --- 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 index 4a3b14a199..51320ade2e 100644 --- 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 index a79f891da7..d768dc0215 100644 --- 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 index 2e42ae115d..9397f72c13 100644 --- 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.