diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 9aef67b..3ca23c8 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -65,6 +65,7 @@ #include "nodes/makefuncs.h" #include "nodes/nodeFuncs.h" #include "nodes/parsenodes.h" +#include "nodes/print.h" #include "optimizer/clauses.h" #include "optimizer/planner.h" #include "optimizer/predtest.h" @@ -13410,7 +13411,6 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs, static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) { - PartitionKey key = RelationGetPartitionKey(rel); Relation attachRel, catalog; List *childrels; @@ -13597,10 +13597,6 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) int num_check = attachRel_constr->num_check; int i; Bitmapset *not_null_attrs = NULL; - List *part_constr; - ListCell *lc; - bool partition_accepts_null = true; - int partnatts; if (attachRel_constr->has_not_null) { @@ -13664,59 +13660,8 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) existConstraint = list_make1(make_ands_explicit(existConstraint)); /* And away we go ... */ - if (predicate_implied_by(partConstraint, existConstraint)) + if (predicate_implied_by(partConstraint, existConstraint, true)) skip_validate = true; - - /* - * We choose to err on the safer side, i.e., give up on skipping the - * validation scan, if the partition key column doesn't have the NOT - * NULL constraint and the table is to become a list partition that - * does not accept nulls. In this case, the partition predicate - * (partConstraint) does include an 'key IS NOT NULL' expression, - * however, because of the way predicate_implied_by_simple_clause() is - * designed to handle IS NOT NULL predicates in the absence of a IS - * NOT NULL clause, we cannot rely on just the above proof. - * - * That is not an issue in case of a range partition, because if there - * were no NOT NULL constraint defined on the key columns, an error - * would be thrown before we get here anyway. That is not true, - * however, if any of the partition keys is an expression, which is - * handled below. - */ - part_constr = linitial(partConstraint); - part_constr = make_ands_implicit((Expr *) part_constr); - - /* - * part_constr contains an IS NOT NULL expression, if this is a list - * partition that does not accept nulls (in fact, also if this is a - * range partition and some partition key is an expression, but we - * never skip validation in that case anyway; see below) - */ - foreach(lc, part_constr) - { - Node *expr = lfirst(lc); - - if (IsA(expr, NullTest) && - ((NullTest *) expr)->nulltesttype == IS_NOT_NULL) - { - partition_accepts_null = false; - break; - } - } - - partnatts = get_partition_natts(key); - for (i = 0; i < partnatts; i++) - { - AttrNumber partattno; - - partattno = get_partition_col_attnum(key, i); - - /* If partition key is an expression, must not skip validation */ - if (!partition_accepts_null && - (partattno == 0 || - !bms_is_member(partattno, not_null_attrs))) - skip_validate = false; - } } /* It's safe to skip the validation scan after all */ diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c index 607a8f9..07ab339 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -1210,10 +1210,10 @@ build_paths_for_OR(PlannerInfo *root, RelOptInfo *rel, all_clauses = list_concat(list_copy(clauses), other_clauses); - if (!predicate_implied_by(index->indpred, all_clauses)) + if (!predicate_implied_by(index->indpred, all_clauses, false)) continue; /* can't use it at all */ - if (!predicate_implied_by(index->indpred, other_clauses)) + if (!predicate_implied_by(index->indpred, other_clauses, false)) useful_predicate = true; } } @@ -1519,7 +1519,7 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel, List *paths) { Node *np = (Node *) lfirst(l); - if (predicate_implied_by(list_make1(np), qualsofar)) + if (predicate_implied_by(list_make1(np), qualsofar, false)) { redundant = true; break; /* out of inner foreach loop */ @@ -2871,7 +2871,8 @@ check_index_predicates(PlannerInfo *root, RelOptInfo *rel) continue; /* ignore non-partial indexes here */ if (!index->predOK) /* don't repeat work if already proven OK */ - index->predOK = predicate_implied_by(index->indpred, clauselist); + index->predOK = predicate_implied_by(index->indpred, clauselist, + false); /* If rel is an update target, leave indrestrictinfo as set above */ if (is_target_rel) @@ -2886,7 +2887,7 @@ check_index_predicates(PlannerInfo *root, RelOptInfo *rel) /* predicate_implied_by() assumes first arg is immutable */ if (contain_mutable_functions((Node *) rinfo->clause) || !predicate_implied_by(list_make1(rinfo->clause), - index->indpred)) + index->indpred, false)) index->indrestrictinfo = lappend(index->indrestrictinfo, rinfo); } } diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 94beeb8..344caf4 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -2576,7 +2576,7 @@ create_indexscan_plan(PlannerInfo *root, if (is_redundant_derived_clause(rinfo, indexquals)) continue; /* derived from same EquivalenceClass */ if (!contain_mutable_functions((Node *) rinfo->clause) && - predicate_implied_by(list_make1(rinfo->clause), indexquals)) + predicate_implied_by(list_make1(rinfo->clause), indexquals, false)) continue; /* provably implied by indexquals */ qpqual = lappend(qpqual, rinfo); } @@ -2737,7 +2737,7 @@ create_bitmap_scan_plan(PlannerInfo *root, if (rinfo->parent_ec && list_member_ptr(indexECs, rinfo->parent_ec)) continue; /* derived from same EquivalenceClass */ if (!contain_mutable_functions(clause) && - predicate_implied_by(list_make1(clause), indexquals)) + predicate_implied_by(list_make1(clause), indexquals, false)) continue; /* provably implied by indexquals */ qpqual = lappend(qpqual, rinfo); } @@ -2968,7 +2968,8 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual, * the conditions that got pushed into the bitmapqual. Avoid * generating redundant conditions. */ - if (!predicate_implied_by(list_make1(pred), ipath->indexclauses)) + if (!predicate_implied_by(list_make1(pred), ipath->indexclauses, + false)) { *qual = lappend(*qual, pred); *indexqual = lappend(*indexqual, pred); diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 8f9dd90..1a775b2 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -776,7 +776,7 @@ infer_arbiter_indexes(PlannerInfo *root) */ predExprs = RelationGetIndexPredicate(idxRel); - if (!predicate_implied_by(predExprs, (List *) onconflict->arbiterWhere)) + if (!predicate_implied_by(predExprs, (List *) onconflict->arbiterWhere, false)) goto next; results = lappend_oid(results, idxForm->indexrelid); diff --git a/src/backend/optimizer/util/predtest.c b/src/backend/optimizer/util/predtest.c index c4a04cf..3a5dc31 100644 --- a/src/backend/optimizer/util/predtest.c +++ b/src/backend/optimizer/util/predtest.c @@ -77,7 +77,8 @@ typedef struct PredIterInfoData } while (0) -static bool predicate_implied_by_recurse(Node *clause, Node *predicate); +static bool predicate_implied_by_recurse(Node *clause, Node *predicate, + bool restrictinfo_is_check); static bool predicate_refuted_by_recurse(Node *clause, Node *predicate); static PredClass predicate_classify(Node *clause, PredIterInfo info); static void list_startup_fn(Node *clause, PredIterInfo info); @@ -90,7 +91,8 @@ static void arrayconst_cleanup_fn(PredIterInfo info); static void arrayexpr_startup_fn(Node *clause, PredIterInfo info); static Node *arrayexpr_next_fn(PredIterInfo info); static void arrayexpr_cleanup_fn(PredIterInfo info); -static bool predicate_implied_by_simple_clause(Expr *predicate, Node *clause); +static bool predicate_implied_by_simple_clause(Expr *predicate, Node *clause, + bool restrictinfo_is_check); static bool predicate_refuted_by_simple_clause(Expr *predicate, Node *clause); static Node *extract_not_arg(Node *clause); static Node *extract_strong_not_arg(Node *clause); @@ -108,7 +110,10 @@ static void InvalidateOprProofCacheCallBack(Datum arg, int cacheid, uint32 hashv /* * predicate_implied_by * Recursively checks whether the clauses in restrictinfo_list imply - * that the given predicate is true. + * that the given predicate is true. If restrictioninfo_is_check is + * true, assume that the clauses in restrictinfo_list are CHECK + * constraints (where null is effectively true) rather than WHERE + * clauses (where null is effectively false). * * The top-level List structure of each list corresponds to an AND list. * We assume that eval_const_expressions() has been applied and so there @@ -125,7 +130,8 @@ static void InvalidateOprProofCacheCallBack(Datum arg, int cacheid, uint32 hashv * the plan and the time we execute the plan. */ bool -predicate_implied_by(List *predicate_list, List *restrictinfo_list) +predicate_implied_by(List *predicate_list, List *restrictinfo_list, + bool restrictinfo_is_check) { Node *p, *r; @@ -151,7 +157,7 @@ predicate_implied_by(List *predicate_list, List *restrictinfo_list) r = (Node *) restrictinfo_list; /* And away we go ... */ - return predicate_implied_by_recurse(r, p); + return predicate_implied_by_recurse(r, p, restrictinfo_is_check); } /* @@ -248,7 +254,8 @@ predicate_refuted_by(List *predicate_list, List *restrictinfo_list) *---------- */ static bool -predicate_implied_by_recurse(Node *clause, Node *predicate) +predicate_implied_by_recurse(Node *clause, Node *predicate, + bool restrictinfo_is_check) { PredIterInfoData clause_info; PredIterInfoData pred_info; @@ -275,7 +282,8 @@ predicate_implied_by_recurse(Node *clause, Node *predicate) result = true; iterate_begin(pitem, predicate, pred_info) { - if (!predicate_implied_by_recurse(clause, pitem)) + if (!predicate_implied_by_recurse(clause, pitem, + restrictinfo_is_check)) { result = false; break; @@ -294,7 +302,8 @@ predicate_implied_by_recurse(Node *clause, Node *predicate) result = false; iterate_begin(pitem, predicate, pred_info) { - if (predicate_implied_by_recurse(clause, pitem)) + if (predicate_implied_by_recurse(clause, pitem, + restrictinfo_is_check)) { result = true; break; @@ -311,7 +320,8 @@ predicate_implied_by_recurse(Node *clause, Node *predicate) */ iterate_begin(citem, clause, clause_info) { - if (predicate_implied_by_recurse(citem, predicate)) + if (predicate_implied_by_recurse(citem, predicate, + restrictinfo_is_check)) { result = true; break; @@ -328,7 +338,8 @@ predicate_implied_by_recurse(Node *clause, Node *predicate) result = false; iterate_begin(citem, clause, clause_info) { - if (predicate_implied_by_recurse(citem, predicate)) + if (predicate_implied_by_recurse(citem, predicate, + restrictinfo_is_check)) { result = true; break; @@ -355,7 +366,8 @@ predicate_implied_by_recurse(Node *clause, Node *predicate) iterate_begin(pitem, predicate, pred_info) { - if (predicate_implied_by_recurse(citem, pitem)) + if (predicate_implied_by_recurse(citem, pitem, + restrictinfo_is_check)) { presult = true; break; @@ -382,7 +394,8 @@ predicate_implied_by_recurse(Node *clause, Node *predicate) result = true; iterate_begin(citem, clause, clause_info) { - if (!predicate_implied_by_recurse(citem, predicate)) + if (!predicate_implied_by_recurse(citem, predicate, + restrictinfo_is_check)) { result = false; break; @@ -404,7 +417,8 @@ predicate_implied_by_recurse(Node *clause, Node *predicate) result = true; iterate_begin(pitem, predicate, pred_info) { - if (!predicate_implied_by_recurse(clause, pitem)) + if (!predicate_implied_by_recurse(clause, pitem, + restrictinfo_is_check)) { result = false; break; @@ -421,7 +435,8 @@ predicate_implied_by_recurse(Node *clause, Node *predicate) result = false; iterate_begin(pitem, predicate, pred_info) { - if (predicate_implied_by_recurse(clause, pitem)) + if (predicate_implied_by_recurse(clause, pitem, + restrictinfo_is_check)) { result = true; break; @@ -437,7 +452,8 @@ predicate_implied_by_recurse(Node *clause, Node *predicate) */ return predicate_implied_by_simple_clause((Expr *) predicate, - clause); + clause, + restrictinfo_is_check); } break; } @@ -558,7 +574,7 @@ predicate_refuted_by_recurse(Node *clause, Node *predicate) */ not_arg = extract_not_arg(predicate); if (not_arg && - predicate_implied_by_recurse(clause, not_arg)) + predicate_implied_by_recurse(clause, not_arg, false)) return true; /* @@ -634,7 +650,7 @@ predicate_refuted_by_recurse(Node *clause, Node *predicate) */ not_arg = extract_not_arg(predicate); if (not_arg && - predicate_implied_by_recurse(clause, not_arg)) + predicate_implied_by_recurse(clause, not_arg, false)) return true; /* @@ -712,7 +728,7 @@ predicate_refuted_by_recurse(Node *clause, Node *predicate) */ not_arg = extract_not_arg(predicate); if (not_arg && - predicate_implied_by_recurse(clause, not_arg)) + predicate_implied_by_recurse(clause, not_arg, false)) return true; /* @@ -1022,14 +1038,15 @@ arrayexpr_cleanup_fn(PredIterInfo info) * functions in the expression are immutable, ie dependent only on their input * arguments --- but this was checked for the predicate by the caller.) * - * When the predicate is of the form "foo IS NOT NULL", we can conclude that - * the predicate is implied if the clause is a strict operator or function - * that has "foo" as an input. In this case the clause must yield NULL when - * "foo" is NULL, which we can take as equivalent to FALSE because we know - * we are within an AND/OR subtree of a WHERE clause. (Again, "foo" is - * already known immutable, so the clause will certainly always fail.) - * Also, if the clause is just "foo" (meaning it's a boolean variable), - * the predicate is implied since the clause can't be true if "foo" is NULL. + * When restrictinfo_is_check is false, we know we are within an AND/OR + * subtree of a WHERE clause. So, if the predicate is of the form "foo IS + * NOT NULL", we can conclude that the predicate is implied if the clause is + * a strict operator or function that has "foo" as an input. In this case + * the clause must yield NULL when "foo" is NULL, which we can take as + * equivalent to FALSE given the context. (Again, "foo" is already known + * immutable, so the clause will certainly always fail.) Also, if the clause + * is just "foo" (meaning it's a boolean variable), the predicate is implied + * since the clause can't be true if "foo" is NULL. * * Finally, if both clauses are binary operator expressions, we may be able * to prove something using the system's knowledge about operators; those @@ -1037,7 +1054,8 @@ arrayexpr_cleanup_fn(PredIterInfo info) *---------- */ static bool -predicate_implied_by_simple_clause(Expr *predicate, Node *clause) +predicate_implied_by_simple_clause(Expr *predicate, Node *clause, + bool restrictinfo_is_check) { /* Allow interrupting long proof attempts */ CHECK_FOR_INTERRUPTS(); @@ -1047,7 +1065,7 @@ predicate_implied_by_simple_clause(Expr *predicate, Node *clause) return true; /* Next try the IS NOT NULL case */ - if (predicate && IsA(predicate, NullTest) && + if (predicate && !restrictinfo_is_check && IsA(predicate, NullTest) && ((NullTest *) predicate)->nulltesttype == IS_NOT_NULL) { Expr *nonnullarg = ((NullTest *) predicate)->arg; diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index 300a8ff..22dabf5 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -6671,7 +6671,7 @@ add_predicate_to_quals(IndexOptInfo *index, List *indexQuals) Node *predQual = (Node *) lfirst(lc); List *oneQual = list_make1(predQual); - if (!predicate_implied_by(oneQual, indexQuals)) + if (!predicate_implied_by(oneQual, indexQuals, false)) predExtraQuals = list_concat(predExtraQuals, oneQual); } /* list_concat avoids modifying the passed-in indexQuals list */ @@ -7556,7 +7556,7 @@ gincostestimate(PlannerInfo *root, IndexPath *path, double loop_count, Node *predQual = (Node *) lfirst(l); List *oneQual = list_make1(predQual); - if (!predicate_implied_by(oneQual, indexQuals)) + if (!predicate_implied_by(oneQual, indexQuals, false)) predExtraQuals = list_concat(predExtraQuals, oneQual); } /* list_concat avoids modifying the passed-in indexQuals list */ diff --git a/src/include/optimizer/predtest.h b/src/include/optimizer/predtest.h index 658a86c..d6b32e0 100644 --- a/src/include/optimizer/predtest.h +++ b/src/include/optimizer/predtest.h @@ -18,7 +18,8 @@ extern bool predicate_implied_by(List *predicate_list, - List *restrictinfo_list); + List *restrictinfo_list, + bool restrictinfo_is_check); extern bool predicate_refuted_by(List *predicate_list, List *restrictinfo_list); diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index d4dbe65..13d6a4b 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -3338,6 +3338,12 @@ ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5); ERROR: partition constraint is violated by some row -- delete the faulting row and also add a constraint to skip the scan DELETE FROM part_5_a WHERE a NOT IN (3); +ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 5); +ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5); +INFO: partition constraint for table "part_5" is implied by existing constraints +ALTER TABLE list_parted2 DETACH PARTITION part_5; +ALTER TABLE part_5 DROP CONSTRAINT check_a; +-- scan should again be skipped, even though NOT NULL is now a column property ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a SET NOT NULL; ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5); INFO: partition constraint for table "part_5" is implied by existing constraints diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 001717d..5dd1402 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -2169,9 +2169,14 @@ ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5); -- delete the faulting row and also add a constraint to skip the scan DELETE FROM part_5_a WHERE a NOT IN (3); -ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a SET NOT NULL; +ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IS NOT NULL AND a = 5); ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5); +ALTER TABLE list_parted2 DETACH PARTITION part_5; +ALTER TABLE part_5 DROP CONSTRAINT check_a; +-- scan should again be skipped, even though NOT NULL is now a column property +ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a SET NOT NULL; +ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5); -- check that the table being attached is not already a partition ALTER TABLE list_parted2 ATTACH PARTITION part_2 FOR VALUES IN (2);