Fix handling of inherited check constraints in ALTER COLUMN TYPE (again).

The previous way of reconstructing check constraints was to do a separate
"ALTER TABLE ONLY tab ADD CONSTRAINT" for each table in an inheritance
hierarchy.  However, that way has no hope of reconstructing the check
constraints' own inheritance properties correctly, as pointed out in
bug #13779 from Jan Dirk Zijlstra.  What we should do instead is to do
a regular "ALTER TABLE", allowing recursion, at the topmost table that
has a particular constraint, and then suppress the work queue entries
for inherited instances of the constraint.

Annoyingly, we'd tried to fix this behavior before, in commit 5ed6546cf,
but we failed to notice that it wasn't reconstructing the pg_constraint
field values correctly.

As long as I'm touching pg_get_constraintdef_worker anyway, tweak it to
always schema-qualify the target table name; this seems like useful backup
to the protections installed by commit 5f173040.

In HEAD/9.5, get rid of get_constraint_relation_oids, which is now unused.
(I could alternatively have modified it to also return conislocal, but that
seemed like a pretty single-purpose API, so let's not pretend it has some
other use.)  It's unused in the back branches as well, but I left it in
place just in case some third-party code has decided to use it.

In HEAD/9.5, also rename pg_get_constraintdef_string to
pg_get_constraintdef_command, as the previous name did nothing to explain
what that entry point did differently from others (and its comment was
equally useless).  Again, that change doesn't seem like material for
back-patching.

I did a bit of re-pgindenting in tablecmds.c in HEAD/9.5, as well.

Otherwise, back-patch to all supported branches.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4538aa09fbf2f10ca46ce5e5276c3888ca655ce0..4aea25cd53b655e1f55d461880bcf6bf9bdc24ac 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3058,7 +3058,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
                        break;
                case AT_ReAddConstraint:        /* Re-add pre-existing check constraint */
                        ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
-                                                               false, true, lockmode);
+                                                               true, true, lockmode);
                        break;
                case AT_AddIndexConstraint:             /* ADD CONSTRAINT USING INDEX */
                        ATExecAddIndexConstraint(tab, rel, (IndexStmt *) cmd->def, lockmode);
@@ -5322,13 +5322,6 @@ ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
  * AddRelationNewConstraints would normally assign different names to the
  * child constraints.  To fix that, we must capture the name assigned at
  * the parent table and pass that down.
- *
- * When re-adding a previously existing constraint (during ALTER COLUMN TYPE),
- * we don't need to recurse here, because recursion will be carried out at a
- * higher level; the constraint name issue doesn't apply because the names
- * have already been assigned and are just being re-used.  We need a separate
- * "is_readd" flag for that; just setting recurse=false would result in an
- * error if there are child tables.
  */
 static void
 ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
@@ -5356,7 +5349,8 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
         */
        newcons = AddRelationNewConstraints(rel, NIL,
                                                                                list_make1(copyObject(constr)),
-                                                                               recursing, !recursing);
+                                                                               recursing | is_readd,   /* allow_merge */
+                                                                               !recursing);    /* is_local */
 
        /* Add each constraint to Phase 3's queue */
        foreach(lcon, newcons)
@@ -5392,13 +5386,6 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
        if (newcons == NIL)
                return;
 
-       /*
-        * Also, in a re-add operation, we don't need to recurse (that will be
-        * handled at higher levels).
-        */
-       if (is_readd)
-               return;
-
        /*
         * Propagate to children as appropriate.  Unlike most other ALTER
         * routines, we have to do this one level of recursion at a time; we can't
@@ -7214,11 +7201,31 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
        forboth(oid_item, tab->changedConstraintOids,
                        def_item, tab->changedConstraintDefs)
        {
-               Oid             oldId = lfirst_oid(oid_item);
-               Oid             relid;
-               Oid             confrelid;
+               Oid                     oldId = lfirst_oid(oid_item);
+               HeapTuple       tup;
+               Form_pg_constraint con;
+               Oid                     relid;
+               Oid                     confrelid;
+               bool            conislocal;
+
+               tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId));
+               if (!HeapTupleIsValid(tup))             /* should not happen */
+                       elog(ERROR, "cache lookup failed for constraint %u", oldId);
+               con = (Form_pg_constraint) GETSTRUCT(tup);
+               relid = con->conrelid;
+               confrelid = con->confrelid;
+               conislocal = con->conislocal;
+               ReleaseSysCache(tup);
+
+               /*
+                * If the constraint is inherited (only), we don't want to inject a
+                * new definition here; it'll get recreated when ATAddCheckConstraint
+                * recurses from adding the parent table's constraint.  But we had to
+                * carry the info this far so that we can drop the constraint below.
+                */
+               if (!conislocal)
+                       continue;
 
-               get_constraint_relation_oids(oldId, &relid, &confrelid);
                ATPostAlterTypeParse(relid, confrelid,
                                                         (char *) lfirst(def_item),
                                                         wqueue, lockmode);

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index bf98620460de4c700658b7be2c7ddd6b064cb1d4..0c728b7193a8580d3340be6a632a65401e708db6 100644 (file)
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -248,6 +248,7 @@ static Node *processIndirection(Node *node, deparse_context *context,
 static void printSubscripts(ArrayRef *aref, deparse_context *context);
 static char *get_relation_name(Oid relid);
 static char *generate_relation_name(Oid relid, List *namespaces);
+static char *generate_qualified_relation_name(Oid relid);
 static char *generate_function_name(Oid funcid, int nargs, List *argnames,
                                           Oid *argtypes, bool *is_variadic);
 static char *generate_operator_name(Oid operid, Oid arg1, Oid arg2);
@@ -1085,7 +1086,9 @@ pg_get_constraintdef_ext(PG_FUNCTION_ARGS)
                                                                                                           false, prettyFlags)));
 }
 
-/* Internal version that returns a palloc'd C string */
+/*
+ * Internal version that returns a full ALTER TABLE ... ADD CONSTRAINT command
+ */
 char *
 pg_get_constraintdef_string(Oid constraintId)
 {
@@ -1107,10 +1110,16 @@ pg_get_constraintdef_worker(Oid constraintId, bool fullCommand,
 
        initStringInfo(&buf);
 
-       if (fullCommand && OidIsValid(conForm->conrelid))
+       if (fullCommand)
        {
-               appendStringInfo(&buf, "ALTER TABLE ONLY %s ADD CONSTRAINT %s ",
-                                                generate_relation_name(conForm->conrelid, NIL),
+               /*
+                * Currently, callers want ALTER TABLE (without ONLY) for CHECK
+                * constraints, and other types of constraints don't inherit anyway so
+                * it doesn't matter whether we say ONLY or not.  Someday we might
+                * need to let callers specify whether to put ONLY in the command.
+                */
+               appendStringInfo(&buf, "ALTER TABLE %s ADD CONSTRAINT %s ",
+                                                generate_qualified_relation_name(conForm->conrelid),
                                                 quote_identifier(NameStr(conForm->conname)));
        }
 
@@ -1626,28 +1635,9 @@ pg_get_serial_sequence(PG_FUNCTION_ARGS)
 
        if (OidIsValid(sequenceId))
        {
-               HeapTuple       classtup;
-               Form_pg_class classtuple;
-               char       *nspname;
                char       *result;
 
-               /* Get the sequence's pg_class entry */
-               classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(sequenceId));
-               if (!HeapTupleIsValid(classtup))
-                       elog(ERROR, "cache lookup failed for relation %u", sequenceId);
-               classtuple = (Form_pg_class) GETSTRUCT(classtup);
-
-               /* Get the namespace */
-               nspname = get_namespace_name(classtuple->relnamespace);
-               if (!nspname)
-                       elog(ERROR, "cache lookup failed for namespace %u",
-                                classtuple->relnamespace);
-
-               /* And construct the result string */
-               result = quote_qualified_identifier(nspname,
-                                                                                       NameStr(classtuple->relname));
-
-               ReleaseSysCache(classtup);
+               result = generate_qualified_relation_name(sequenceId);
 
                PG_RETURN_TEXT_P(string_to_text(result));
        }
@@ -7171,6 +7161,39 @@ generate_relation_name(Oid relid, List *namespaces)
        return result;
 }
 
+/*
+ * generate_qualified_relation_name
+ *             Compute the name to display for a relation specified by OID
+ *
+ * As above, but unconditionally schema-qualify the name.
+ */
+static char *
+generate_qualified_relation_name(Oid relid)
+{
+       HeapTuple       tp;
+       Form_pg_class reltup;
+       char       *relname;
+       char       *nspname;
+       char       *result;
+
+       tp = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+       if (!HeapTupleIsValid(tp))
+               elog(ERROR, "cache lookup failed for relation %u", relid);
+       reltup = (Form_pg_class) GETSTRUCT(tp);
+       relname = NameStr(reltup->relname);
+
+       nspname = get_namespace_name(reltup->relnamespace);
+       if (!nspname)
+               elog(ERROR, "cache lookup failed for namespace %u",
+                        reltup->relnamespace);
+
+       result = quote_qualified_identifier(nspname, relname);
+
+       ReleaseSysCache(tp);
+
+       return result;
+}
+
 /*
  * generate_function_name
  *             Compute the name to display for a function specified by OID,

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5c76a162f41abc876665df446f83e319cfea6a13..0509d97c6caef999956a5fa7a0405947bf996614 100644 (file)
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1537,16 +1537,121 @@ where oid = 'test_storage'::regclass;
  t
 (1 row)
 
--- ALTER TYPE with a check constraint and a child table (bug before Nov 2012)
-CREATE TABLE test_inh_check (a float check (a > 10.2));
+-- ALTER COLUMN TYPE with a check constraint and a child table (bug #13779)
+CREATE TABLE test_inh_check (a float check (a > 10.2), b float);
 CREATE TABLE test_inh_check_child() INHERITS(test_inh_check);
+\d test_inh_check
+     Table "public.test_inh_check"
+ Column |       Type       | Modifiers 
+--------+------------------+-----------
+ a      | double precision | 
+ b      | double precision | 
+Check constraints:
+    "test_inh_check_a_check" CHECK (a > 10.2::double precision)
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d test_inh_check_child
+  Table "public.test_inh_check_child"
+ Column |       Type       | Modifiers 
+--------+------------------+-----------
+ a      | double precision | 
+ b      | double precision | 
+Check constraints:
+    "test_inh_check_a_check" CHECK (a > 10.2::double precision)
+Inherits: test_inh_check
+
+select relname, conname, coninhcount, conislocal
+  from pg_constraint c, pg_class r
+  where relname like 'test_inh_check%' and c.conrelid = r.oid
+  order by 1, 2;
+       relname        |        conname         | coninhcount | conislocal 
+----------------------+------------------------+-------------+------------
+ test_inh_check       | test_inh_check_a_check |           0 | t
+ test_inh_check_child | test_inh_check_a_check |           1 | f
+(2 rows)
+
 ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric;
+\d test_inh_check
+     Table "public.test_inh_check"
+ Column |       Type       | Modifiers 
+--------+------------------+-----------
+ a      | numeric          | 
+ b      | double precision | 
+Check constraints:
+    "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d test_inh_check_child
+  Table "public.test_inh_check_child"
+ Column |       Type       | Modifiers 
+--------+------------------+-----------
+ a      | numeric          | 
+ b      | double precision | 
+Check constraints:
+    "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+Inherits: test_inh_check
+
+select relname, conname, coninhcount, conislocal
+  from pg_constraint c, pg_class r
+  where relname like 'test_inh_check%' and c.conrelid = r.oid
+  order by 1, 2;
+       relname        |        conname         | coninhcount | conislocal 
+----------------------+------------------------+-------------+------------
+ test_inh_check       | test_inh_check_a_check |           0 | t
+ test_inh_check_child | test_inh_check_a_check |           1 | f
+(2 rows)
+
+-- also try local and local+inherited cases
+ALTER TABLE test_inh_check_child ADD CONSTRAINT blocal CHECK (b < 1000);
+ALTER TABLE test_inh_check_child ADD CONSTRAINT bmerged CHECK (b > 1);
+ALTER TABLE test_inh_check ADD CONSTRAINT bmerged CHECK (b > 1);
+NOTICE:  merging constraint "bmerged" with inherited definition
+\d test_inh_check
+     Table "public.test_inh_check"
+ Column |       Type       | Modifiers 
+--------+------------------+-----------
+ a      | numeric          | 
+ b      | double precision | 
+Check constraints:
+    "bmerged" CHECK (b > 1::double precision)
+    "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d test_inh_check_child
+  Table "public.test_inh_check_child"
+ Column |       Type       | Modifiers 
+--------+------------------+-----------
+ a      | numeric          | 
+ b      | double precision | 
+Check constraints:
+    "blocal" CHECK (b < 1000::double precision)
+    "bmerged" CHECK (b > 1::double precision)
+    "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+Inherits: test_inh_check
+
+select relname, conname, coninhcount, conislocal
+  from pg_constraint c, pg_class r
+  where relname like 'test_inh_check%' and c.conrelid = r.oid
+  order by 1, 2;
+       relname        |        conname         | coninhcount | conislocal 
+----------------------+------------------------+-------------+------------
+ test_inh_check       | bmerged                |           0 | t
+ test_inh_check       | test_inh_check_a_check |           0 | t
+ test_inh_check_child | blocal                 |           0 | t
+ test_inh_check_child | bmerged                |           1 | t
+ test_inh_check_child | test_inh_check_a_check |           1 | f
+(5 rows)
+
+ALTER TABLE test_inh_check ALTER COLUMN b TYPE numeric;
+NOTICE:  merging constraint "bmerged" with inherited definition
 \d test_inh_check
 Table "public.test_inh_check"
  Column |  Type   | Modifiers 
 --------+---------+-----------
  a      | numeric | 
+ b      | numeric | 
 Check constraints:
+    "bmerged" CHECK (b::double precision > 1::double precision)
     "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
 Number of child tables: 1 (Use \d+ to list them.)
 
@@ -1555,10 +1660,26 @@ Table "public.test_inh_check_child"
  Column |  Type   | Modifiers 
 --------+---------+-----------
  a      | numeric | 
+ b      | numeric | 
 Check constraints:
+    "blocal" CHECK (b::double precision < 1000::double precision)
+    "bmerged" CHECK (b::double precision > 1::double precision)
     "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
 Inherits: test_inh_check
 
+select relname, conname, coninhcount, conislocal
+  from pg_constraint c, pg_class r
+  where relname like 'test_inh_check%' and c.conrelid = r.oid
+  order by 1, 2;
+       relname        |        conname         | coninhcount | conislocal 
+----------------------+------------------------+-------------+------------
+ test_inh_check       | bmerged                |           0 | t
+ test_inh_check       | test_inh_check_a_check |           0 | t
+ test_inh_check_child | blocal                 |           0 | t
+ test_inh_check_child | bmerged                |           1 | t
+ test_inh_check_child | test_inh_check_a_check |           1 | f
+(5 rows)
+
 -- check for rollback of ANALYZE corrupting table property flags (bug #11638)
 CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);
 NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "check_fk_presence_1_pkey" for table "check_fk_presence_1"

diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 6828b0a2cbb0285623fede788b90ded3c4b4c2f8..f567bfa435a9a3045c372c214c8034f615773df6 100644 (file)
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1144,12 +1144,39 @@ select reltoastrelid <> 0 as has_toast_table
 from pg_class
 where oid = 'test_storage'::regclass;
 
--- ALTER TYPE with a check constraint and a child table (bug before Nov 2012)
-CREATE TABLE test_inh_check (a float check (a > 10.2));
+-- ALTER COLUMN TYPE with a check constraint and a child table (bug #13779)
+CREATE TABLE test_inh_check (a float check (a > 10.2), b float);
 CREATE TABLE test_inh_check_child() INHERITS(test_inh_check);
+\d test_inh_check
+\d test_inh_check_child
+select relname, conname, coninhcount, conislocal
+  from pg_constraint c, pg_class r
+  where relname like 'test_inh_check%' and c.conrelid = r.oid
+  order by 1, 2;
 ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric;
 \d test_inh_check
 \d test_inh_check_child
+select relname, conname, coninhcount, conislocal
+  from pg_constraint c, pg_class r
+  where relname like 'test_inh_check%' and c.conrelid = r.oid
+  order by 1, 2;
+-- also try local and local+inherited cases
+ALTER TABLE test_inh_check_child ADD CONSTRAINT blocal CHECK (b < 1000);
+ALTER TABLE test_inh_check_child ADD CONSTRAINT bmerged CHECK (b > 1);
+ALTER TABLE test_inh_check ADD CONSTRAINT bmerged CHECK (b > 1);
+\d test_inh_check
+\d test_inh_check_child
+select relname, conname, coninhcount, conislocal
+  from pg_constraint c, pg_class r
+  where relname like 'test_inh_check%' and c.conrelid = r.oid
+  order by 1, 2;
+ALTER TABLE test_inh_check ALTER COLUMN b TYPE numeric;
+\d test_inh_check
+\d test_inh_check_child
+select relname, conname, coninhcount, conislocal
+  from pg_constraint c, pg_class r
+  where relname like 'test_inh_check%' and c.conrelid = r.oid
+  order by 1, 2;
 
 -- check for rollback of ANALYZE corrupting table property flags (bug #11638)
 CREATE TABLE check_fk_presence_1 (id int PRIMARY KEY, t text);