From: Tom Lane Date: Sat, 8 Oct 2016 23:29:27 +0000 (-0400) Subject: Fix two bugs in merging of inherited CHECK constraints. X-Git-Tag: REL9_6_1~37 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b605aeba0e117e4b8087fc95eba4cce07bb185ef;p=postgresql Fix two bugs in merging of inherited CHECK constraints. Historically, we've allowed users to add a CHECK constraint to a child table and then add an identical CHECK constraint to the parent. This results in "merging" the two constraints so that the pre-existing child constraint ends up with both conislocal = true and coninhcount > 0. However, if you tried to do it in the other order, you got a duplicate constraint error. This is problematic for pg_dump, which needs to issue separated ADD CONSTRAINT commands in some cases, but has no good way to ensure that the constraints will be added in the required order. And it's more than a bit arbitrary, too. The goal of complaining about duplicated ADD CONSTRAINT commands can be served if we reject the case of adding a constraint when the existing one already has conislocal = true; but if it has conislocal = false, let's just make the ADD CONSTRAINT set conislocal = true. In this way, either order of adding the constraints has the same end result. Another problem was that the code allowed creation of a parent constraint marked convalidated that is merged with a child constraint that is !convalidated. In this case, an inheritance scan of the parent table could emit some rows violating the constraint condition, which would be an unexpected result given the marking of the parent constraint as validated. Hence, forbid merging of constraints in this case. (Note: valid child and not-valid parent seems fine, so continue to allow that.) Per report from Benedikt Grundmann. Back-patch to 9.2 where we introduced possibly-not-valid check constraints. The second bug obviously doesn't apply before that, and I think the first doesn't either, because pg_dump only gets into this situation when dealing with not-valid constraints. Report: Discussion: <22108.1475874586@sss.pgh.pa.us> --- diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index e997b574ca..53e8144052 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -105,6 +105,7 @@ static void StoreConstraints(Relation rel, List *cooked_constraints, bool is_internal); static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr, bool allow_merge, bool is_local, + bool is_initially_valid, bool is_no_inherit); static void SetRelationNumChecks(Relation rel, int numchecks); static Node *cookConstraint(ParseState *pstate, @@ -2301,6 +2302,7 @@ AddRelationNewConstraints(Relation rel, */ if (MergeWithExistingConstraint(rel, ccname, expr, allow_merge, is_local, + cdef->initially_valid, cdef->is_no_inherit)) continue; } @@ -2389,6 +2391,7 @@ AddRelationNewConstraints(Relation rel, static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr, bool allow_merge, bool is_local, + bool is_initially_valid, bool is_no_inherit) { bool found; @@ -2436,22 +2439,47 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr, if (equal(expr, stringToNode(TextDatumGetCString(val)))) found = true; } + + /* + * If the existing constraint is purely inherited (no local + * definition) then interpret addition of a local constraint as a + * legal merge. This allows ALTER ADD CONSTRAINT on parent and + * child tables to be given in either order with same end state. + */ + if (is_local && !con->conislocal) + allow_merge = true; + if (!found || !allow_merge) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT), errmsg("constraint \"%s\" for relation \"%s\" already exists", ccname, RelationGetRelationName(rel)))); - tup = heap_copytuple(tup); - con = (Form_pg_constraint) GETSTRUCT(tup); - - /* If the constraint is "no inherit" then cannot merge */ + /* If the child constraint is "no inherit" then cannot merge */ if (con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"", ccname, RelationGetRelationName(rel)))); + /* + * If the child constraint is "not valid" then cannot merge with a + * valid parent constraint + */ + if (is_initially_valid && !con->convalidated) + ereport(ERROR, + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), + errmsg("constraint \"%s\" conflicts with NOT VALID constraint on relation \"%s\"", + ccname, RelationGetRelationName(rel)))); + + /* OK to update the tuple */ + ereport(NOTICE, + (errmsg("merging constraint \"%s\" with inherited definition", + ccname))); + + tup = heap_copytuple(tup); + con = (Form_pg_constraint) GETSTRUCT(tup); + if (is_local) con->conislocal = true; else @@ -2461,10 +2489,6 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr, Assert(is_local); con->connoinherit = true; } - /* OK to update the tuple */ - ereport(NOTICE, - (errmsg("merging constraint \"%s\" with inherited definition", - ccname))); simple_heap_update(conDesc, &tup->t_self, tup); CatalogUpdateIndexes(conDesc, tup); break; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 86e98148c1..0828711691 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -10372,7 +10372,7 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) RelationGetRelationName(child_rel), NameStr(parent_con->conname)))); - /* If the constraint is "no inherit" then cannot merge */ + /* If the child constraint is "no inherit" then cannot merge */ if (child_con->connoinherit) ereport(ERROR, (errcode(ERRCODE_INVALID_OBJECT_DEFINITION), @@ -10380,6 +10380,17 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel) NameStr(child_con->conname), RelationGetRelationName(child_rel)))); + /* + * If the child constraint is "not valid" then cannot merge with a + * valid parent constraint + */ + 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\"", + NameStr(child_con->conname), + RelationGetRelationName(child_rel)))); + /* * OK, bump the child constraint's inheritance count. (If we fail * later on, this change will just roll back.) diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index d8b5b1d44e..9d374fe6c4 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1090,6 +1090,56 @@ Inherits: test_foreign_constraints DROP TABLE test_foreign_constraints_inh; DROP TABLE test_foreign_constraints; DROP TABLE test_primary_constraints; +-- Test that parent and child CHECK constraints can be created in either order +create table p1(f1 int); +create table p1_c1() inherits(p1); +alter table p1 add constraint inh_check_constraint1 check (f1 > 0); +alter table p1_c1 add constraint inh_check_constraint1 check (f1 > 0); +NOTICE: merging constraint "inh_check_constraint1" with inherited definition +alter table p1_c1 add constraint inh_check_constraint2 check (f1 < 10); +alter table p1 add constraint inh_check_constraint2 check (f1 < 10); +NOTICE: merging constraint "inh_check_constraint2" with inherited definition +select conrelid::regclass::text as relname, conname, conislocal, coninhcount +from pg_constraint where conname like 'inh\_check\_constraint%' +order by 1, 2; + relname | conname | conislocal | coninhcount +---------+-----------------------+------------+------------- + p1 | inh_check_constraint1 | t | 0 + p1 | inh_check_constraint2 | t | 0 + p1_c1 | inh_check_constraint1 | t | 1 + p1_c1 | inh_check_constraint2 | t | 1 +(4 rows) + +drop table p1 cascade; +NOTICE: drop cascades to table p1_c1 +-- Test that a valid child can have not-valid parent, but not vice versa +create table invalid_check_con(f1 int); +create table invalid_check_con_child() inherits(invalid_check_con); +alter table invalid_check_con_child add constraint inh_check_constraint check(f1 > 0) not valid; +alter table invalid_check_con add constraint inh_check_constraint check(f1 > 0); -- fail +ERROR: constraint "inh_check_constraint" conflicts with NOT VALID constraint on relation "invalid_check_con_child" +alter table invalid_check_con_child drop constraint inh_check_constraint; +insert into invalid_check_con values(0); +alter table invalid_check_con_child add constraint inh_check_constraint check(f1 > 0); +alter table invalid_check_con add constraint inh_check_constraint check(f1 > 0) not valid; +NOTICE: merging constraint "inh_check_constraint" with inherited definition +insert into invalid_check_con values(0); -- fail +ERROR: new row for relation "invalid_check_con" violates check constraint "inh_check_constraint" +DETAIL: Failing row contains (0). +insert into invalid_check_con_child values(0); -- fail +ERROR: new row for relation "invalid_check_con_child" violates check constraint "inh_check_constraint" +DETAIL: Failing row contains (0). +select conrelid::regclass::text as relname, conname, + convalidated, conislocal, coninhcount, connoinherit +from pg_constraint where conname like 'inh\_check\_constraint%' +order by 1, 2; + relname | conname | convalidated | conislocal | coninhcount | connoinherit +-------------------------+----------------------+--------------+------------+-------------+-------------- + invalid_check_con | inh_check_constraint | f | t | 0 | f + invalid_check_con_child | inh_check_constraint | t | t | 1 | f +(2 rows) + +-- We don't drop the invalid_check_con* tables, to test dump/reload with -- -- Test parameterized append plans for inheritance trees -- diff --git a/src/test/regress/expected/sanity_check.out b/src/test/regress/expected/sanity_check.out index 1c087a3903..b1ebcf60d2 100644 --- a/src/test/regress/expected/sanity_check.out +++ b/src/test/regress/expected/sanity_check.out @@ -62,6 +62,8 @@ int2_tbl|f int4_tbl|f int8_tbl|f interval_tbl|f +invalid_check_con|f +invalid_check_con_child|f iportaltest|f kd_point_tbl|t line_tbl|f diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index b307a5049b..6b1df754a6 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -334,6 +334,45 @@ DROP TABLE test_foreign_constraints_inh; DROP TABLE test_foreign_constraints; DROP TABLE test_primary_constraints; +-- Test that parent and child CHECK constraints can be created in either order +create table p1(f1 int); +create table p1_c1() inherits(p1); + +alter table p1 add constraint inh_check_constraint1 check (f1 > 0); +alter table p1_c1 add constraint inh_check_constraint1 check (f1 > 0); + +alter table p1_c1 add constraint inh_check_constraint2 check (f1 < 10); +alter table p1 add constraint inh_check_constraint2 check (f1 < 10); + +select conrelid::regclass::text as relname, conname, conislocal, coninhcount +from pg_constraint where conname like 'inh\_check\_constraint%' +order by 1, 2; + +drop table p1 cascade; + +-- Test that a valid child can have not-valid parent, but not vice versa +create table invalid_check_con(f1 int); +create table invalid_check_con_child() inherits(invalid_check_con); + +alter table invalid_check_con_child add constraint inh_check_constraint check(f1 > 0) not valid; +alter table invalid_check_con add constraint inh_check_constraint check(f1 > 0); -- fail +alter table invalid_check_con_child drop constraint inh_check_constraint; + +insert into invalid_check_con values(0); + +alter table invalid_check_con_child add constraint inh_check_constraint check(f1 > 0); +alter table invalid_check_con add constraint inh_check_constraint check(f1 > 0) not valid; + +insert into invalid_check_con values(0); -- fail +insert into invalid_check_con_child values(0); -- fail + +select conrelid::regclass::text as relname, conname, + convalidated, conislocal, coninhcount, connoinherit +from pg_constraint where conname like 'inh\_check\_constraint%' +order by 1, 2; + +-- We don't drop the invalid_check_con* tables, to test dump/reload with + -- -- Test parameterized append plans for inheritance trees --