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: <CADbMkNPT-Jz5PRSQ4RbUASYAjocV_KHUWapR%2Bg8fNvhUAyRpxA%40mail.gmail.com>
Discussion: <22108.1475874586@sss.pgh.pa.us>

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index ea266a8519ac7504f26b851254d06a08f10233a9..3ba43a3edaf8bdc820a162e14d4c9fdcf400b06e 100644 (file)
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -97,6 +97,7 @@ static void StoreRelCheck(Relation rel, char *ccname, Node *expr,
 static void StoreConstraints(Relation rel, List *cooked_constraints);
 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,
@@ -2183,6 +2184,7 @@ AddRelationNewConstraints(Relation rel,
                         */
                        if (MergeWithExistingConstraint(rel, ccname, expr,
                                                                                        allow_merge, is_local,
+                                                                                       cdef->initially_valid,
                                                                                        cdef->is_no_inherit))
                                continue;
                }
@@ -2271,6 +2273,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;
@@ -2318,22 +2321,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
@@ -2343,10 +2371,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 0c89d81707ba32eaa29b32503ac0051af8f4a8d8..4854385cf1cda26ffbdaa7b621f9db1c06c49ca4 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9327,7 +9327,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),
@@ -9335,6 +9335,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 145715101142c6345b69faf622675ffde3b2ce8c..fa3d9029417493ac4741e95ef59371986ef2a6d2 100644 (file)
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1107,6 +1107,56 @@ Has OIDs: no
 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 7f560d2a4d7b3a9848f2c09fc800b5eadd792870..2489f61ce1f57f05a8ba263c01b652af48460a7b 100644 (file)
--- a/src/test/regress/expected/sanity_check.out
+++ b/src/test/regress/expected/sanity_check.out
@@ -62,6 +62,8 @@ SELECT relname, relhasindex
  int4_tbl                | f
  int8_tbl                | f
  interval_tbl            | f
+ invalid_check_con       | f
+ invalid_check_con_child | f
  iportaltest             | f
  kd_point_tbl            | t
  log_table               | f
@@ -164,7 +166,7 @@ SELECT relname, relhasindex
  timetz_tbl              | f
  tinterval_tbl           | f
  varchar_tbl             | f
-(153 rows)
+(155 rows)
 
 --
 -- another sanity check: every system catalog that has OIDs should have

diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index c85931c1f2c17d9fce1e2068547d33cf07bf775f..b64221f7eeabd7b114fbdcc404fbf3d3d4833026 100644 (file)
--- 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
 --