Fix handling of inherited check constraints in ALTER COLUMN TYPE.

This case got broken in 8.4 by the addition of an error check that
complains if ALTER TABLE ONLY is used on a table that has children.
We do use ONLY for this situation, but it's okay because the necessary
recursion occurs at a higher level.  So we need to have a separate
flag to suppress recursion without making the error check.

Reported and patched by Pavan Deolasee, with some editorial adjustments by
me.  Back-patch to 8.4, since this is a regression of functionality that
worked in earlier branches.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1bd9bef3e723e47b66d24c5a413137f007a84208..f88bf793e57e60f98a73fcf1de5a750d34cdb5b0 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -335,13 +335,15 @@ static void ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
               IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode);
 static void ATExecAddConstraint(List **wqueue,
                    AlteredTableInfo *tab, Relation rel,
-                Constraint *newConstraint, bool recurse, LOCKMODE lockmode);
+                   Constraint *newConstraint, bool recurse, bool is_readd,
+                   LOCKMODE lockmode);
 static void ATExecAddIndexConstraint(AlteredTableInfo *tab, Relation rel,
                         IndexStmt *stmt, LOCKMODE lockmode);
 static void ATAddCheckConstraint(List **wqueue,
                     AlteredTableInfo *tab, Relation rel,
                     Constraint *constr,
-                    bool recurse, bool recursing, LOCKMODE lockmode);
+                    bool recurse, bool recursing, bool is_readd,
+                    LOCKMODE lockmode);
 static void ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
                          Constraint *fkconstraint, LOCKMODE lockmode);
 static void ATExecDropConstraint(Relation rel, const char *constrName,
@@ -2758,6 +2760,7 @@ AlterTableGetLockLevel(List *cmds)
            case AT_ColumnDefault:
            case AT_ProcessedConstraint:        /* becomes AT_AddConstraint */
            case AT_AddConstraintRecurse:       /* becomes AT_AddConstraint */
+           case AT_ReAddConstraint:            /* becomes AT_AddConstraint */
            case AT_EnableTrig:
            case AT_EnableAlwaysTrig:
            case AT_EnableReplicaTrig:
@@ -3249,11 +3252,15 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
            break;
        case AT_AddConstraint:  /* ADD CONSTRAINT */
            ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
-                               false, lockmode);
+                               false, false, lockmode);
            break;
        case AT_AddConstraintRecurse:   /* ADD CONSTRAINT with recursion */
            ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
-                               true, lockmode);
+                               true, false, lockmode);
+           break;
+       case AT_ReAddConstraint:    /* Re-add pre-existing check constraint */
+           ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
+                               false, true, lockmode);
            break;
        case AT_AddIndexConstraint:     /* ADD CONSTRAINT USING INDEX */
            ATExecAddIndexConstraint(tab, rel, (IndexStmt *) cmd->def, lockmode);
@@ -5500,7 +5507,8 @@ ATExecAddIndexConstraint(AlteredTableInfo *tab, Relation rel,
  */
 static void
 ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
-                 Constraint *newConstraint, bool recurse, LOCKMODE lockmode)
+                   Constraint *newConstraint, bool recurse, bool is_readd,
+                   LOCKMODE lockmode)
 {
    Assert(IsA(newConstraint, Constraint));
 
@@ -5513,7 +5521,8 @@ ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
    {
        case CONSTR_CHECK:
            ATAddCheckConstraint(wqueue, tab, rel,
-                                newConstraint, recurse, false, lockmode);
+                                newConstraint, recurse, false, is_readd,
+                                lockmode);
            break;
 
        case CONSTR_FOREIGN:
@@ -5565,11 +5574,18 @@ 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,
                     Constraint *constr, bool recurse, bool recursing,
-                    LOCKMODE lockmode)
+                    bool is_readd, LOCKMODE lockmode)
 {
    List       *newcons;
    ListCell   *lcon;
@@ -5634,9 +5650,11 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
        return;
 
    /*
-    * Adding a NO INHERIT constraint? No need to find our children
+    * If adding a NO INHERIT constraint, no need to find our children.
+    * Likewise, in a re-add operation, we don't need to recurse (that will be
+    * handled at higher levels).
     */
-   if (constr->is_no_inherit)
+   if (constr->is_no_inherit || is_readd)
        return;
 
    /*
@@ -5671,7 +5689,7 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
 
        /* Recurse to child */
        ATAddCheckConstraint(wqueue, childtab, childrel,
-                            constr, recurse, true, lockmode);
+                            constr, recurse, true, is_readd, lockmode);
 
        heap_close(childrel, NoLock);
    }
@@ -7862,6 +7880,10 @@ ATPostAlterTypeParse(Oid oldId, char *cmd,
    /*
     * Attach each generated command to the proper place in the work queue.
     * Note this could result in creation of entirely new work-queue entries.
+    *
+    * Also note that we have to tweak the command subtypes, because it turns
+    * out that re-creation of indexes and constraints has to act a bit
+    * differently from initial creation.
     */
    foreach(list_item, querytree_list)
    {
@@ -7919,6 +7941,7 @@ ATPostAlterTypeParse(Oid oldId, char *cmd,
                                if (con->contype == CONSTR_FOREIGN &&
                                    !rewrite && !tab->rewrite)
                                    TryReuseForeignKey(oldId, con);
+                               cmd->subtype = AT_ReAddConstraint;
                                tab->subcmds[AT_PASS_OLD_CONSTR] =
                                    lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
                                break;

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 09b15e7694cde1932b3640a8bb8f69e7606ea6b9..88344990f47e04578a3d267d930d588e7aab5cd4 100644 (file)
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1197,6 +1197,7 @@ typedef enum AlterTableType
    AT_ReAddIndex,              /* internal to commands/tablecmds.c */
    AT_AddConstraint,           /* add constraint */
    AT_AddConstraintRecurse,    /* internal to commands/tablecmds.c */
+   AT_ReAddConstraint,         /* internal to commands/tablecmds.c */
    AT_ValidateConstraint,      /* validate constraint */
    AT_ValidateConstraintRecurse,       /* internal to commands/tablecmds.c */
    AT_ProcessedConstraint,     /* pre-processed add constraint (local in

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index c22d74c7b562e86d6e839b4ab3a87d3cf7a29149..8392c27b368d6db70a1c9d379487d27eb86c844a 100644 (file)
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1780,6 +1780,28 @@ 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));
+CREATE TABLE test_inh_check_child() INHERITS(test_inh_check);
+ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric;
+\d test_inh_check
+Table "public.test_inh_check"
+ Column |  Type   | Modifiers 
+--------+---------+-----------
+ a      | numeric | 
+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 | 
+Check constraints:
+    "test_inh_check_a_check" CHECK (a::double precision > 10.2::double precision)
+Inherits: test_inh_check
+
 --
 -- lock levels
 --

diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 0f9cb380e1662749295cc2fca959346a8d1b84c6..dcf8121d70c1dd7ac2db2ffaf58ce20bca2ec742 100644 (file)
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1239,6 +1239,13 @@ 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));
+CREATE TABLE test_inh_check_child() INHERITS(test_inh_check);
+ALTER TABLE test_inh_check ALTER COLUMN a TYPE numeric;
+\d test_inh_check
+\d test_inh_check_child
+
 --
 -- lock levels
 --