Fix ALTER TABLE ONLY .. DROP CONSTRAINT.

When I consolidated two copies of the HOT-chain search logic in commit
4da99ea4231e3d8bbf28b666748c1028e7b7d665, I introduced a behavior
change: the old code wouldn't necessarily traverse the entire chain,
if the most recently returned tuple were updated while the HOT chain
traversal is in progress.  The new behavior seems more correct, but
unfortunately, the code here relies on a scan with SnapshotNow failing
to see its own updates.  That seems pretty shaky even with the old HOT
chain traversal behavior, since there's no guarantee that these
updates will always be HOT, but it's trivial to broke a failure with
the new HOT search logic.  Fix by updating just the first matching
pg_constraint tuple, rather than all of them, since there should be
only one anyway.  But since nobody has reproduced this failure on older
versions, no back-patch for now.

Report and test case by Alex Hunsaker; tablecmds.c changes by me.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1e8ad2b671682ad21c372f845ad5454f5998ed84..7b241fd15da1c0a601cb3efdbe995fc4ee4f46ff 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6734,6 +6734,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
    {
        Oid         childrelid = lfirst_oid(child);
        Relation    childrel;
+       HeapTuple   copy_tuple;
 
        /* find_inheritance_children already got lock */
        childrel = heap_open(childrelid, NoLock);
@@ -6746,83 +6747,79 @@ ATExecDropConstraint(Relation rel, const char *constrName,
        scan = systable_beginscan(conrel, ConstraintRelidIndexId,
                                  true, SnapshotNow, 1, &key);
 
-       found = false;
-
+       /* scan for matching tuple - there should only be one */
        while (HeapTupleIsValid(tuple = systable_getnext(scan)))
        {
-           HeapTuple   copy_tuple;
-
            con = (Form_pg_constraint) GETSTRUCT(tuple);
 
            /* Right now only CHECK constraints can be inherited */
            if (con->contype != CONSTRAINT_CHECK)
                continue;
 
-           if (strcmp(NameStr(con->conname), constrName) != 0)
-               continue;
+           if (strcmp(NameStr(con->conname), constrName) == 0)
+               break;
+       }
 
-           found = true;
+       if (!HeapTupleIsValid(tuple))
+           ereport(ERROR,
+                   (errcode(ERRCODE_UNDEFINED_OBJECT),
+               errmsg("constraint \"%s\" of relation \"%s\" does not exist",
+                      constrName,
+                      RelationGetRelationName(childrel))));
 
-           if (con->coninhcount <= 0)  /* shouldn't happen */
-               elog(ERROR, "relation %u has non-inherited constraint \"%s\"",
-                    childrelid, constrName);
+       copy_tuple = heap_copytuple(tuple);
 
-           copy_tuple = heap_copytuple(tuple);
-           con = (Form_pg_constraint) GETSTRUCT(copy_tuple);
+       systable_endscan(scan);
 
-           if (recurse)
-           {
-               /*
-                * If the child constraint has other definition sources, just
-                * decrement its inheritance count; if not, recurse to delete
-                * it.
-                */
-               if (con->coninhcount == 1 && !con->conislocal)
-               {
-                   /* Time to delete this child constraint, too */
-                   ATExecDropConstraint(childrel, constrName, behavior,
-                                        true, true,
-                                        false, lockmode);
-               }
-               else
-               {
-                   /* Child constraint must survive my deletion */
-                   con->coninhcount--;
-                   simple_heap_update(conrel, &copy_tuple->t_self, copy_tuple);
-                   CatalogUpdateIndexes(conrel, copy_tuple);
+       con = (Form_pg_constraint) GETSTRUCT(copy_tuple);
 
-                   /* Make update visible */
-                   CommandCounterIncrement();
-               }
+       if (con->coninhcount <= 0)  /* shouldn't happen */
+           elog(ERROR, "relation %u has non-inherited constraint \"%s\"",
+                childrelid, constrName);
+
+       if (recurse)
+       {
+           /*
+            * If the child constraint has other definition sources, just
+            * decrement its inheritance count; if not, recurse to delete
+            * it.
+            */
+           if (con->coninhcount == 1 && !con->conislocal)
+           {
+               /* Time to delete this child constraint, too */
+               ATExecDropConstraint(childrel, constrName, behavior,
+                                    true, true,
+                                    false, lockmode);
            }
            else
            {
-               /*
-                * If we were told to drop ONLY in this table (no recursion),
-                * we need to mark the inheritors' constraints as locally
-                * defined rather than inherited.
-                */
+               /* Child constraint must survive my deletion */
                con->coninhcount--;
-               con->conislocal = true;
-
                simple_heap_update(conrel, &copy_tuple->t_self, copy_tuple);
                CatalogUpdateIndexes(conrel, copy_tuple);
 
                /* Make update visible */
                CommandCounterIncrement();
            }
-
-           heap_freetuple(copy_tuple);
        }
+       else
+       {
+           /*
+            * If we were told to drop ONLY in this table (no recursion),
+            * we need to mark the inheritors' constraints as locally
+            * defined rather than inherited.
+            */
+           con->coninhcount--;
+           con->conislocal = true;
 
-       systable_endscan(scan);
+           simple_heap_update(conrel, &copy_tuple->t_self, copy_tuple);
+           CatalogUpdateIndexes(conrel, copy_tuple);
 
-       if (!found)
-           ereport(ERROR,
-                   (errcode(ERRCODE_UNDEFINED_OBJECT),
-               errmsg("constraint \"%s\" of relation \"%s\" does not exist",
-                      constrName,
-                      RelationGetRelationName(childrel))));
+           /* Make update visible */
+           CommandCounterIncrement();
+       }
+
+       heap_freetuple(copy_tuple);
 
        heap_close(childrel, NoLock);
    }

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 005a88b084b66259db091b64f0d5f0ec7e43b102..1aa4f09ed28687954180c1594d295fcbfad8f8bf 100644 (file)
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2103,3 +2103,12 @@ ALTER TABLE tt7 NOT OF;
  x      | integer      | 
  y      | numeric(8,2) | 
 
+-- make sure we can drop a constraint on the parent but it remains on the child
+CREATE TABLE test_drop_constr_parent (c text CHECK (c IS NOT NULL));
+CREATE TABLE test_drop_constr_child () INHERITS (test_drop_constr_parent);
+ALTER TABLE ONLY test_drop_constr_parent DROP CONSTRAINT "test_drop_constr_parent_c_check";
+-- should fail
+INSERT INTO test_drop_constr_child (c) VALUES (NULL);
+ERROR:  new row for relation "test_drop_constr_child" violates check constraint "test_drop_constr_parent_c_check"
+DROP TABLE test_drop_constr_parent CASCADE;
+NOTICE:  drop cascades to table test_drop_constr_child

diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 95e898c0eac743b5318d8ad2ff14bcb432460e94..a477f0401b4651de7fdad6cef10c5bd55abf2830 100644 (file)
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1456,3 +1456,11 @@ CREATE TYPE tt_t1 AS (x int, y numeric(8,2));
 ALTER TABLE tt7 OF tt_t1;          -- reassign an already-typed table
 ALTER TABLE tt7 NOT OF;
 \d tt7
+
+-- make sure we can drop a constraint on the parent but it remains on the child
+CREATE TABLE test_drop_constr_parent (c text CHECK (c IS NOT NULL));
+CREATE TABLE test_drop_constr_child () INHERITS (test_drop_constr_parent);
+ALTER TABLE ONLY test_drop_constr_parent DROP CONSTRAINT "test_drop_constr_parent_c_check";
+-- should fail
+INSERT INTO test_drop_constr_child (c) VALUES (NULL);
+DROP TABLE test_drop_constr_parent CASCADE;