Code review for recent patch to allow ALTER TABLE ADD COLUMN when

a child table already has a matching column.  Acquire appropriate
lock on child table; do the right thing with any CHECK constraints
attached to the new parent column.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f211448f413c6cdf00a856088a2b6f525d48a96f..62d61b315c65a32e4af79e7f76e0c2d23b759b16 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v 1.50 2002/10/21 22:06:19 tgl Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v 1.51 2002/11/02 22:02:08 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1649,6 +1649,7 @@ AlterTableAddColumn(Oid myrelid,
                                   *children;
                ColumnDef  *colDefChild = copyObject(colDef);
 
+               /* Child should see column as singly inherited */
                colDefChild->inhcount = 1;
                colDefChild->is_local = false;
 
@@ -1665,37 +1666,53 @@ AlterTableAddColumn(Oid myrelid,
                        if (childrelid == myrelid)
                                continue;
 
+                       childrel = heap_open(childrelid, AccessExclusiveLock);
+
+                       /* Does child already have a column by this name? */
                        attrdesc = heap_openr(AttributeRelationName, RowExclusiveLock);
                        tuple = SearchSysCacheCopyAttName(childrelid, colDef->colname);
                        if (!HeapTupleIsValid(tuple))
                        {
+                               /* No, recurse to add it normally */
                                heap_close(attrdesc, RowExclusiveLock);
+                               heap_close(childrel, NoLock);
                                AlterTableAddColumn(childrelid, true, colDefChild);
                                continue;
                        }
                        childatt = (Form_pg_attribute) GETSTRUCT(tuple);
 
-                       typeTuple = typenameType(colDef->typename);
+                       /* Okay if child matches by type */
+                       if (typenameTypeId(colDef->typename) != childatt->atttypid ||
+                               colDef->typename->typmod != childatt->atttypmod)
+                               elog(ERROR, "ALTER TABLE: child table \"%s\" has different type for column \"%s\"",
+                                        get_rel_name(childrelid), colDef->colname);
 
-                       if (HeapTupleGetOid(typeTuple) != childatt->atttypid ||
-                                       colDef->typename->typmod != childatt->atttypmod)
-                               elog(ERROR, "ALTER TABLE: child table %u has different "
-                                               "type for column \"%s\"",
-                                               childrelid, colDef->colname);
+                       /*
+                        * XXX if we supported NOT NULL or defaults, would need to do
+                        * more work here to verify child matches
+                        */
 
+                       elog(NOTICE, "ALTER TABLE: merging definition of column \"%s\" for child %s",
+                                colDef->colname, get_rel_name(childrelid));
+
+                       /* Bump the existing child att's inhcount */
                        childatt->attinhcount++;
                        simple_heap_update(attrdesc, &tuple->t_self, tuple);
                        CatalogUpdateIndexes(attrdesc, tuple);
-                       
-                       childrel = RelationIdGetRelation(childrelid);
-                       elog(NOTICE, "ALTER TABLE: merging definition of column "
-                                       "\"%s\" for child %s", colDef->colname,
-                                       RelationGetRelationName(childrel));
-                       RelationClose(childrel);
 
-                       heap_close(attrdesc, RowExclusiveLock);
+                       /*
+                        * Propagate any new CHECK constraints into the child table
+                        * and its descendants
+                        */
+                       if (colDef->constraints != NIL)
+                       {
+                               CommandCounterIncrement();
+                               AlterTableAddConstraint(childrelid, true, colDef->constraints);
+                       }
+
                        heap_freetuple(tuple);
-                       ReleaseSysCache(typeTuple);
+                       heap_close(attrdesc, RowExclusiveLock);
+                       heap_close(childrel, NoLock);
                }
        }
        else