From f99a766a2bfad9af9b90d731ca9f0ba0cdc7d1fe Mon Sep 17 00:00:00 2001 From: Amul Sul Date: Fri, 7 Feb 2025 09:41:20 +0530 Subject: [PATCH v16 7/9] Ease the restriction that a NOT ENFORCED constraint must be INVALID. In the initial support for NOT ENFORCED check constraints in commit ca87c415e2fccf81cec6fd45698dde9fae0ab570, we introduced a restriction that a NOT ENFORCED constraint must be NOT VALID. However, we still mark a NOT ENFORCED constraint as NOT VALID when adding it to an already existing table, as validation cannot be performed on a NOT ENFORCED constraint. Conversely, when the constraint is created along with the table, it is acceptable to mark the constraint as valid. --- src/backend/catalog/heap.c | 3 +- src/backend/catalog/pg_constraint.c | 2 - src/backend/commands/tablecmds.c | 19 ++--- src/backend/optimizer/util/plancat.c | 9 +-- src/backend/parser/gram.y | 9 --- src/backend/parser/parse_utilcmd.c | 13 ++-- src/backend/utils/adt/ruleutils.c | 6 +- src/test/regress/expected/alter_table.out | 4 +- src/test/regress/expected/inherit.out | 88 ++++++++++++++--------- src/test/regress/sql/alter_table.sql | 3 +- src/test/regress/sql/inherit.sql | 13 +++- 11 files changed, 84 insertions(+), 85 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index bd3554c0bfd..96ed3824088 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2791,7 +2791,7 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr, * If the child constraint is "not valid" then cannot merge with a * valid parent constraint. */ - if (is_initially_valid && con->conenforced && !con->convalidated) + if (is_initially_valid && !con->convalidated) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with NOT VALID constraint on relation \"%s\"", @@ -2854,7 +2854,6 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr, { Assert(is_local); con->conenforced = true; - con->convalidated = true; } CatalogTupleUpdate(conDesc, &tup->t_self, tup); diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c index ac80652baf2..60bc54a96b6 100644 --- a/src/backend/catalog/pg_constraint.c +++ b/src/backend/catalog/pg_constraint.c @@ -102,8 +102,6 @@ CreateConstraintEntry(const char *constraintName, /* Only CHECK constraint can be not enforced */ Assert(isEnforced || constraintType == CONSTRAINT_CHECK); - /* NOT ENFORCED constraint must be NOT VALID */ - Assert(isEnforced || !isValidated); conDesc = table_open(ConstraintRelationId, RowExclusiveLock); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d8561fad53e..5b2b284002e 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3157,10 +3157,7 @@ MergeCheckConstraint(List *constraints, const char *name, Node *expr, bool is_en * marked as ENFORCED because one of the parents is ENFORCED. */ if (!ccon->is_enforced && is_enforced) - { ccon->is_enforced = true; - ccon->skip_validation = false; - } return constraints; } @@ -3181,7 +3178,6 @@ MergeCheckConstraint(List *constraints, const char *name, Node *expr, bool is_en newcon->expr = expr; newcon->inhcount = 1; newcon->is_enforced = is_enforced; - newcon->skip_validation = !is_enforced; return lappend(constraints, newcon); } @@ -17010,8 +17006,7 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) * If the child constraint is "not valid" then cannot merge with a * valid parent constraint */ - if (parent_con->convalidated && child_con->conenforced && - !child_con->convalidated) + if (parent_con->convalidated && !child_con->convalidated) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with NOT VALID constraint on child table \"%s\"", @@ -19379,18 +19374,12 @@ ConstraintImpliedByRelConstraint(Relation scanrel, List *testConstraint, List *p Node *cexpr; /* - * If this constraint hasn't been fully validated yet, we must ignore - * it here. + * If this constraint hasn't been fully validated yet or is not + * enforced, we must ignore it here. */ - if (!constr->check[i].ccvalid) + if (!constr->check[i].ccvalid || !constr->check[i].ccenforced) continue; - /* - * NOT ENFORCED constraints are always marked as invalid, which should - * have been ignored. - */ - Assert(constr->check[i].ccenforced); - cexpr = stringToNode(constr->check[i].ccbin); /* diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c index 71abb01f655..233a5d8da98 100644 --- a/src/backend/optimizer/util/plancat.c +++ b/src/backend/optimizer/util/plancat.c @@ -1302,22 +1302,15 @@ get_relation_constraints(PlannerInfo *root, * ignore it here. Also ignore if NO INHERIT and we weren't told * that that's safe. */ - if (!constr->check[i].ccvalid) + if (!constr->check[i].ccvalid || !constr->check[i].ccenforced) continue; - /* - * NOT ENFORCED constraints are always marked as invalid, which - * should have been ignored. - */ - Assert(constr->check[i].ccenforced); - /* * Also ignore if NO INHERIT and we weren't told that that's safe. */ if (constr->check[i].ccnoinherit && !include_noinherit) continue; - cexpr = stringToNode(constr->check[i].ccbin); /* diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 271ae26cbaf..042fbbdc078 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -19587,15 +19587,6 @@ processCASbits(int cas_bits, int location, const char *constrType, errmsg("%s constraints cannot be marked NOT ENFORCED", constrType), parser_errposition(location))); - - /* - * NB: The validated status is irrelevant when the constraint is set to - * NOT ENFORCED, but for consistency, it should be set accordingly. - * This ensures that if the constraint is later changed to ENFORCED, it - * will automatically be in the correct NOT VALIDATED state. - */ - if (not_valid) - *not_valid = true; } if (cas_bits & CAS_ENFORCED) diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index abbe1bb45a3..37becb62356 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -1453,7 +1453,6 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause) char *ccname = constr->check[ccnum].ccname; char *ccbin = constr->check[ccnum].ccbin; bool ccenforced = constr->check[ccnum].ccenforced; - bool ccvalid = constr->check[ccnum].ccvalid; bool ccnoinherit = constr->check[ccnum].ccnoinherit; Node *ccbin_node; bool found_whole_row; @@ -1484,13 +1483,13 @@ expandTableLikeClause(RangeVar *heapRel, TableLikeClause *table_like_clause) n->conname = pstrdup(ccname); n->location = -1; n->is_enforced = ccenforced; - n->initially_valid = ccvalid; n->is_no_inherit = ccnoinherit; n->raw_expr = NULL; n->cooked_expr = nodeToString(ccbin_node); /* We can skip validation, since the new table should be empty. */ n->skip_validation = true; + n->initially_valid = true; atsubcmd = makeNode(AlterTableCmd); atsubcmd->subtype = AT_AddConstraint; @@ -2944,11 +2943,9 @@ transformCheckConstraints(CreateStmtContext *cxt, bool skipValidation) return; /* - * When creating a new table (but not a foreign table), we can safely skip - * the validation of check constraints and mark them as valid based on the - * constraint enforcement flag, since NOT ENFORCED constraints must always - * be marked as NOT VALID. (This will override any user-supplied NOT VALID - * flag.) + * If creating a new table (but not a foreign table), we can safely skip + * validation of check constraints, and nonetheless mark them valid. (This + * will override any user-supplied NOT VALID flag.) */ if (skipValidation) { @@ -2957,7 +2954,7 @@ transformCheckConstraints(CreateStmtContext *cxt, bool skipValidation) Constraint *constraint = (Constraint *) lfirst(ckclist); constraint->skip_validation = true; - constraint->initially_valid = constraint->is_enforced; + constraint->initially_valid = true; } } } diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index d11a8a20eea..f709ded1136 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -2594,12 +2594,10 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand, appendStringInfoString(&buf, " DEFERRABLE"); if (conForm->condeferred) appendStringInfoString(&buf, " INITIALLY DEFERRED"); - - /* Validated status is irrelevant when the constraint is NOT ENFORCED. */ + if (!conForm->convalidated) + appendStringInfoString(&buf, " NOT VALID"); if (!conForm->conenforced) appendStringInfoString(&buf, " NOT ENFORCED"); - else if (!conForm->convalidated) - appendStringInfoString(&buf, " NOT VALID"); /* Cleanup */ systable_endscan(scandesc); diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 362f38856d2..54bdb3f2250 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -507,7 +507,9 @@ ALTER TABLE attmp3 validate constraint attmpconstr; ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten CHECK (b > 10); -- fail ERROR: check constraint "b_greater_than_ten" of relation "attmp3" is violated by some row ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten CHECK (b > 10) NOT VALID; -- succeeds -ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK (b > 10) NOT ENFORCED; -- succeeds +ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK (b > 10) NOT ENFORCED; -- fail +ERROR: check constraint "b_greater_than_ten_not_enforced" of relation "attmp3" is violated by some row +ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK (b > 10) NOT VALID NOT ENFORCED; -- succeeds ALTER TABLE attmp3 VALIDATE CONSTRAINT b_greater_than_ten; -- fails ERROR: check constraint "b_greater_than_ten" of relation "attmp3" is violated by some row DELETE FROM attmp3 WHERE NOT b > 10; diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index e671975a281..70e4830e032 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1332,6 +1332,18 @@ NOTICE: merging constraint "inh_check_constraint5" with inherited definition alter table p1 add constraint inh_check_constraint6 check (f1 < 10) not enforced; alter table p1_c1 add constraint inh_check_constraint6 check (f1 < 10) enforced; NOTICE: merging constraint "inh_check_constraint6" with inherited definition +-- however, cannot merge a not valid enforced child constraint with a valid +-- not-enforced parent constraint. +alter table p1_c1 add constraint inh_check_constraint9 check (f1 < 10) not valid enforced; +alter table p1 add constraint inh_check_constraint9 check (f1 < 10) not enforced; +ERROR: constraint "inh_check_constraint9" conflicts with NOT VALID constraint on relation "p1_c1" +-- but, allowed if the parent constraint is also invalid. +alter table p1 add constraint inh_check_constraint9 check (f1 < 10) not valid not enforced; +NOTICE: merging constraint "inh_check_constraint9" with inherited definition +-- the invalid state of the child constraint will be ignored here. +alter table p1 add constraint inh_check_constraint10 check (f1 < 10) not enforced; +alter table p1_c1 add constraint inh_check_constraint10 check (f1 < 10) not valid enforced; +NOTICE: merging constraint "inh_check_constraint10" with inherited definition create table p1_c2(f1 int constraint inh_check_constraint4 check (f1 < 10)) inherits(p1); NOTICE: merging column "f1" with inherited definition NOTICE: merging constraint "inh_check_constraint4" with inherited definition @@ -1353,42 +1365,50 @@ create table p1_fail(f1 int constraint inh_check_constraint6 check (f1 < 10) not NOTICE: merging multiple inherited definitions of column "f1" NOTICE: merging column "f1" with inherited definition ERROR: constraint "inh_check_constraint6" conflicts with NOT ENFORCED constraint on relation "p1_fail" -select conrelid::regclass::text as relname, conname, conislocal, coninhcount, conenforced +select conrelid::regclass::text as relname, conname, conislocal, coninhcount, conenforced, convalidated from pg_constraint where conname like 'inh\_check\_constraint%' order by 1, 2; - relname | conname | conislocal | coninhcount | conenforced ----------+-----------------------+------------+-------------+------------- - p1 | inh_check_constraint1 | t | 0 | t - p1 | inh_check_constraint2 | t | 0 | t - p1 | inh_check_constraint3 | t | 0 | f - p1 | inh_check_constraint4 | t | 0 | f - p1 | inh_check_constraint5 | t | 0 | f - p1 | inh_check_constraint6 | t | 0 | f - p1 | inh_check_constraint8 | t | 0 | t - p1_c1 | inh_check_constraint1 | t | 1 | t - p1_c1 | inh_check_constraint2 | t | 1 | t - p1_c1 | inh_check_constraint3 | t | 1 | f - p1_c1 | inh_check_constraint4 | t | 1 | f - p1_c1 | inh_check_constraint5 | t | 1 | t - p1_c1 | inh_check_constraint6 | t | 1 | t - p1_c1 | inh_check_constraint7 | t | 0 | f - p1_c1 | inh_check_constraint8 | f | 1 | t - p1_c2 | inh_check_constraint1 | f | 1 | t - p1_c2 | inh_check_constraint2 | f | 1 | t - p1_c2 | inh_check_constraint3 | f | 1 | f - p1_c2 | inh_check_constraint4 | t | 1 | t - p1_c2 | inh_check_constraint5 | f | 1 | f - p1_c2 | inh_check_constraint6 | f | 1 | f - p1_c2 | inh_check_constraint8 | f | 1 | t - p1_c3 | inh_check_constraint1 | f | 2 | t - p1_c3 | inh_check_constraint2 | f | 2 | t - p1_c3 | inh_check_constraint3 | f | 2 | f - p1_c3 | inh_check_constraint4 | f | 2 | f - p1_c3 | inh_check_constraint5 | f | 2 | t - p1_c3 | inh_check_constraint6 | f | 2 | t - p1_c3 | inh_check_constraint7 | f | 1 | f - p1_c3 | inh_check_constraint8 | f | 2 | t -(30 rows) + relname | conname | conislocal | coninhcount | conenforced | convalidated +---------+------------------------+------------+-------------+-------------+-------------- + p1 | inh_check_constraint1 | t | 0 | t | t + p1 | inh_check_constraint10 | t | 0 | f | t + p1 | inh_check_constraint2 | t | 0 | t | t + p1 | inh_check_constraint3 | t | 0 | f | t + p1 | inh_check_constraint4 | t | 0 | f | t + p1 | inh_check_constraint5 | t | 0 | f | t + p1 | inh_check_constraint6 | t | 0 | f | t + p1 | inh_check_constraint8 | t | 0 | t | t + p1 | inh_check_constraint9 | t | 0 | f | f + p1_c1 | inh_check_constraint1 | t | 1 | t | t + p1_c1 | inh_check_constraint10 | t | 1 | t | t + p1_c1 | inh_check_constraint2 | t | 1 | t | t + p1_c1 | inh_check_constraint3 | t | 1 | f | t + p1_c1 | inh_check_constraint4 | t | 1 | f | t + p1_c1 | inh_check_constraint5 | t | 1 | t | t + p1_c1 | inh_check_constraint6 | t | 1 | t | t + p1_c1 | inh_check_constraint7 | t | 0 | f | t + p1_c1 | inh_check_constraint8 | f | 1 | t | t + p1_c1 | inh_check_constraint9 | t | 1 | t | f + p1_c2 | inh_check_constraint1 | f | 1 | t | t + p1_c2 | inh_check_constraint10 | f | 1 | f | t + p1_c2 | inh_check_constraint2 | f | 1 | t | t + p1_c2 | inh_check_constraint3 | f | 1 | f | t + p1_c2 | inh_check_constraint4 | t | 1 | t | t + p1_c2 | inh_check_constraint5 | f | 1 | f | t + p1_c2 | inh_check_constraint6 | f | 1 | f | t + p1_c2 | inh_check_constraint8 | f | 1 | t | t + p1_c2 | inh_check_constraint9 | f | 1 | f | t + p1_c3 | inh_check_constraint1 | f | 2 | t | t + p1_c3 | inh_check_constraint10 | f | 2 | t | t + p1_c3 | inh_check_constraint2 | f | 2 | t | t + p1_c3 | inh_check_constraint3 | f | 2 | f | t + p1_c3 | inh_check_constraint4 | f | 2 | f | t + p1_c3 | inh_check_constraint5 | f | 2 | t | t + p1_c3 | inh_check_constraint6 | f | 2 | t | t + p1_c3 | inh_check_constraint7 | f | 1 | f | t + p1_c3 | inh_check_constraint8 | f | 2 | t | t + p1_c3 | inh_check_constraint9 | f | 2 | t | t +(38 rows) drop table p1 cascade; NOTICE: drop cascades to 3 other objects diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 84e93ef575e..a9268de14b5 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -387,7 +387,8 @@ ALTER TABLE attmp3 validate constraint attmpconstr; -- Try a non-verified CHECK constraint ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten CHECK (b > 10); -- fail ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten CHECK (b > 10) NOT VALID; -- succeeds -ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK (b > 10) NOT ENFORCED; -- succeeds +ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK (b > 10) NOT ENFORCED; -- fail +ALTER TABLE attmp3 ADD CONSTRAINT b_greater_than_ten_not_enforced CHECK (b > 10) NOT VALID NOT ENFORCED; -- succeeds ALTER TABLE attmp3 VALIDATE CONSTRAINT b_greater_than_ten; -- fails DELETE FROM attmp3 WHERE NOT b > 10; ALTER TABLE attmp3 VALIDATE CONSTRAINT b_greater_than_ten; -- succeeds diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 4e73c70495c..5169456181b 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -481,6 +481,17 @@ alter table p1 add constraint inh_check_constraint5 check (f1 < 10) not enforced alter table p1 add constraint inh_check_constraint6 check (f1 < 10) not enforced; alter table p1_c1 add constraint inh_check_constraint6 check (f1 < 10) enforced; +-- however, cannot merge a not valid enforced child constraint with a valid +-- not-enforced parent constraint. +alter table p1_c1 add constraint inh_check_constraint9 check (f1 < 10) not valid enforced; +alter table p1 add constraint inh_check_constraint9 check (f1 < 10) not enforced; +-- but, allowed if the parent constraint is also invalid. +alter table p1 add constraint inh_check_constraint9 check (f1 < 10) not valid not enforced; + +-- the invalid state of the child constraint will be ignored here. +alter table p1 add constraint inh_check_constraint10 check (f1 < 10) not enforced; +alter table p1_c1 add constraint inh_check_constraint10 check (f1 < 10) not valid enforced; + create table p1_c2(f1 int constraint inh_check_constraint4 check (f1 < 10)) inherits(p1); -- but reverse is not allowed @@ -498,7 +509,7 @@ create table p1_c3() inherits(p1, p1_c1); -- but not allowed if the child constraint is explicitly asked to be NOT ENFORCED create table p1_fail(f1 int constraint inh_check_constraint6 check (f1 < 10) not enforced) inherits(p1, p1_c1); -select conrelid::regclass::text as relname, conname, conislocal, coninhcount, conenforced +select conrelid::regclass::text as relname, conname, conislocal, coninhcount, conenforced, convalidated from pg_constraint where conname like 'inh\_check\_constraint%' order by 1, 2; -- 2.43.5