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 8993fc007c91bd010166121ab16a580b71208464..9102b050e8128b38c6601ccce13517a42f50f9c9 100644 (file)
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -101,6 +101,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,
@@ -2262,6 +2263,7 @@ AddRelationNewConstraints(Relation rel,
             */
            if (MergeWithExistingConstraint(rel, ccname, expr,
                                            allow_merge, is_local,
+                                           cdef->initially_valid,
                                            cdef->is_no_inherit))
                continue;
        }
@@ -2350,6 +2352,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;
@@ -2397,22 +2400,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
@@ -2422,10 +2450,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 f2a7f9e888ade84f1703d352e9a3afbcc0f1bb13..7e2a6787929ba9307c52ce574fc1a0c94f61504b 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -9420,7 +9420,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),
@@ -9428,6 +9428,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 67fb580e304e164566c0057c60691a5a1de0e8af..a274813bff2952a43a99539c601bce6745512059 100644 (file)
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1103,6 +1103,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 432d39a4911ffaa7107a9011d584b6be1c61c8d7..66c21e8c174c27cd18c50e155a55cc5bb0eb519c 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
@@ -166,7 +168,7 @@ SELECT relname, relhasindex
  timetz_tbl              | f
  tinterval_tbl           | f
  varchar_tbl             | f
-(155 rows)
+(157 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 793c216376858a69b3a0c3cfb7f662f0347a8b3c..a5e6ecdb868287a2700db43af069cef8484682ba 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
 --