Thread: ADD/DROP INHERITS

ADD/DROP INHERITS

From
Greg Stark
Date:
This is where I am with the ADD/DROP INHERITS patch now.

1) The syntax is:

   ALTER TABLE child INHERIT parent
   ALTER TABLE child NO INHERIT parent

   no new reserved words, no conflicts, no complicated grammar productions in
   gram.y to confuse people in the future.

2) Dependencies are being added and dropped by trawling directly through
   pg_depend rather than having dependencies on pg_depend lines.

3) Constraints are being compared by reverse-compiling the definition and
   comparing the result with strcmp.

   I also haven't checked the constraint name. To do so it would make sense to
   use a small hash table. I see something called dynahash in the source tree.
   Is it meant for these kind of quick small tasks?

   I'm ignoring unique, primary key, and foreign key constraints on the theory
   that these things don't really work on inherited tables yet anyways. Also
   NONE of these are copied when you create a new inherited table so it would
   mean that you wouldn't be able to re-add a freshly automatically generated
   child after removing it.

   Actually a foreign key constraint *from* a child table work fine. But like
   I said it currently isn't being copied when you create a child table. We
   could consider fixing that and adding a check here for matching foreign key
   constraints at the same time.




Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.184
diff -u -p -c -r1.184 tablecmds.c
cvs diff: conflicting specifications of output style
*** src/backend/commands/tablecmds.c    10 May 2006 23:18:39 -0000    1.184
--- src/backend/commands/tablecmds.c    9 Jun 2006 21:28:13 -0000
*************** typedef struct NewColumnValue
*** 159,166 ****
--- 159,168 ----
  static void truncate_check_rel(Relation rel);
  static List *MergeAttributes(List *schema, List *supers, bool istemp,
                  List **supOids, List **supconstr, int *supOidCount);
+ static void MergeAttributesIntoExisting(Relation rel, Relation relation);
  static bool change_varattnos_of_a_node(Node *node, const AttrNumber *newattno);
  static void StoreCatalogInheritance(Oid relationId, List *supers);
+ static void StoreCatalogInheritance1(Oid relationId, Oid parentOid, int16 seqNumber, Relation catalogRelation);
  static int    findAttrByName(const char *attributeName, List *schema);
  static void setRelhassubclassInRelation(Oid relationId, bool relhassubclass);
  static bool needs_toast_table(Relation rel);
*************** static void ATPrepSetTableSpace(AlteredT
*** 246,251 ****
--- 248,255 ----
  static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace);
  static void ATExecEnableDisableTrigger(Relation rel, char *trigname,
                             bool enable, bool skip_system);
+ static void ATExecAddInherits(Relation rel, RangeVar *parent);
+ static void ATExecDropInherits(Relation rel, RangeVar *parent);
  static void copy_relation_data(Relation rel, SMgrRelation dst);
  static void update_ri_trigger_args(Oid relid,
                         const char *oldname,
*************** static void
*** 1156,1165 ****
  StoreCatalogInheritance(Oid relationId, List *supers)
  {
      Relation    relation;
-     TupleDesc    desc;
      int16        seqNumber;
      ListCell   *entry;
-     HeapTuple    tuple;

      /*
       * sanity checks
--- 1160,1167 ----
*************** StoreCatalogInheritance(Oid relationId,
*** 1179,1194 ****
       * anymore, there's no need to look for indirect ancestors.)
       */
      relation = heap_open(InheritsRelationId, RowExclusiveLock);
-     desc = RelationGetDescr(relation);

      seqNumber = 1;
      foreach(entry, supers)
      {
!         Oid            parentOid = lfirst_oid(entry);
          Datum        datum[Natts_pg_inherits];
          char        nullarr[Natts_pg_inherits];
          ObjectAddress childobject,
                      parentobject;

          datum[0] = ObjectIdGetDatum(relationId);        /* inhrel */
          datum[1] = ObjectIdGetDatum(parentOid); /* inhparent */
--- 1181,1206 ----
       * anymore, there's no need to look for indirect ancestors.)
       */
      relation = heap_open(InheritsRelationId, RowExclusiveLock);

      seqNumber = 1;
      foreach(entry, supers)
      {
!         StoreCatalogInheritance1(relationId, lfirst_oid(entry), seqNumber, relation);
!         seqNumber += 1;
!     }
!
!     heap_close(relation, RowExclusiveLock);
! }
!
! static void
! StoreCatalogInheritance1(Oid relationId, Oid parentOid, int16 seqNumber, Relation relation)
! {
          Datum        datum[Natts_pg_inherits];
          char        nullarr[Natts_pg_inherits];
          ObjectAddress childobject,
                      parentobject;
+         HeapTuple    tuple;
+         TupleDesc desc = RelationGetDescr(relation);

          datum[0] = ObjectIdGetDatum(relationId);        /* inhrel */
          datum[1] = ObjectIdGetDatum(parentOid); /* inhparent */
*************** StoreCatalogInheritance(Oid relationId,
*** 1223,1232 ****
           */
          setRelhassubclassInRelation(parentOid, true);

-         seqNumber += 1;
-     }
-
-     heap_close(relation, RowExclusiveLock);
  }

  /*
--- 1235,1240 ----
*************** ATPrepCmd(List **wqueue, Relation rel, A
*** 2053,2058 ****
--- 2061,2068 ----
          case AT_DisableTrig:    /* DISABLE TRIGGER variants */
          case AT_DisableTrigAll:
          case AT_DisableTrigUser:
+         case AT_AddInherits:
+         case AT_DropInherits:
              ATSimplePermissions(rel, false);
              /* These commands never recurse */
              /* No command-specific prep needed */
*************** ATExecCmd(AlteredTableInfo *tab, Relatio
*** 2233,2238 ****
--- 2243,2254 ----
          case AT_DisableTrigUser:        /* DISABLE TRIGGER USER */
              ATExecEnableDisableTrigger(rel, NULL, false, true);
              break;
+         case AT_DropInherits:
+             ATExecDropInherits(rel, cmd->parent);
+             break;
+         case AT_AddInherits:
+             ATExecAddInherits(rel, cmd->parent);
+             break;
          default:                /* oops */
              elog(ERROR, "unrecognized alter table type: %d",
                   (int) cmd->subtype);
*************** ATExecEnableDisableTrigger(Relation rel,
*** 5880,5885 ****
--- 5896,6274 ----
      EnableDisableTrigger(rel, trigname, enable, skip_system);
  }

+ static bool
+ list_member_string(List *list, void *datum)
+ {
+     ListCell   *cell;
+
+     foreach(cell, list) {
+         if (!strcmp(lfirst(cell), datum))
+             return true;
+     }
+     return false;
+ }
+
+ static void
+ ATExecAddInherits(Relation rel, RangeVar *parent)
+ {
+     Relation     relation, catalogRelation;
+     SysScanDesc scan;
+     ScanKeyData key;
+     HeapTuple     inheritsTuple, constraintTuple;
+     int4         inhseqno;
+     ListCell       *child;
+     List         *children, *constraints;
+     TupleDesc       tupleDesc;
+
+     relation = heap_openrv(parent, AccessShareLock); /* XXX is this enough locking? */
+     if (relation->rd_rel->relkind != RELKIND_RELATION)
+         ereport(ERROR,
+                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                  errmsg("inherited relation \"%s\" is not a table",
+                         parent->relname)));
+
+
+     /* Permanent rels cannot inherit from temporary ones */
+     if (!rel->rd_istemp && isTempNamespace(RelationGetNamespace(relation)))
+         ereport(ERROR,
+                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                  errmsg("cannot inherit from temporary relation \"%s\"",
+                         parent->relname)));
+
+     if (!pg_class_ownercheck(RelationGetRelid(relation), GetUserId()))
+         aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
+                        RelationGetRelationName(relation));
+
+     /* If parent has OIDs then all children must have OIDs */
+     if (relation->rd_rel->relhasoids && !rel->rd_rel->relhasoids)
+         ereport(ERROR,
+                 (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                  errmsg("table \"%s\" without OIDs cannot inherit from table \"%s\" with OIDs",
+                         RelationGetRelationName(rel), parent->relname)));
+
+     /*
+      * Reject duplications in the list of parents. -- this is the same check as
+      * when creating a table, but maybe we should check for the parent anywhere
+      * higher in the inheritance structure?
+      */
+     catalogRelation = heap_open(InheritsRelationId, RowExclusiveLock);
+     ScanKeyInit(&key,
+                 Anum_pg_inherits_inhrelid,
+                 BTEqualStrategyNumber, F_OIDEQ,
+                 ObjectIdGetDatum(RelationGetRelid(rel)));
+     scan = systable_beginscan(catalogRelation, InheritsRelidSeqnoIndexId, true, SnapshotNow, 1, &key);
+     inhseqno = 0; /* inhseqno sequences are supposed to start at 1 */
+     while (HeapTupleIsValid(inheritsTuple = systable_getnext(scan)))
+     {
+         Form_pg_inherits inh = (Form_pg_inherits) GETSTRUCT(inheritsTuple);
+         if (inh->inhparent == RelationGetRelid(relation))
+             ereport(ERROR,
+                     (errcode(ERRCODE_DUPLICATE_TABLE),
+                      errmsg("inherited relation \"%s\" duplicated",
+                             parent->relname)));
+         /* Accumulate the max seqno -- BUT if we spot a hole use it instead
+          * This means if you DROP and ADD the same parent the order will be maintained
+          * which will mean pg_dump won't gratuitously reorder your columns.
+          */
+         if (inh->inhseqno == inhseqno+1)
+             inhseqno = inh->inhseqno;
+     }
+     systable_endscan(scan);
+     heap_close(catalogRelation, RowExclusiveLock);
+
+     /* Get children because we have to manually recurse and also because we
+      * have to check for recursive inheritance graphs */
+
+     /* this routine is actually in the planner */
+     children = find_all_inheritors(RelationGetRelid(rel));
+
+     if (list_member_oid(children, RelationGetRelid(relation)))
+         ereport(ERROR,
+                 (errcode(ERRCODE_DUPLICATE_TABLE),
+                  errmsg("circular inheritance structure found, \"%s\" is already a child of \"%s\"",
+                         parent->relname, RelationGetRelationName(rel))));
+
+     foreach(child, children)
+         {
+             Oid            childrelid = lfirst_oid(child);
+             Relation    childrel;
+
+             childrel = relation_open(childrelid, AccessExclusiveLock);
+             MergeAttributesIntoExisting(childrel, relation);
+             relation_close(childrel, NoLock);
+         }
+
+
+
+     /* Now check the constraints */
+     /* First gather up the child's constraint definitions */
+     catalogRelation = heap_open(ConstraintRelationId, AccessShareLock);
+     tupleDesc = RelationGetDescr(catalogRelation);
+
+     ScanKeyInit(&key,
+                 Anum_pg_constraint_conrelid,
+                 BTEqualStrategyNumber, F_OIDEQ,
+                 ObjectIdGetDatum(RelationGetRelid(rel)));
+     scan = systable_beginscan(catalogRelation, ConstraintRelidIndexId, true, SnapshotNow, 1, &key);
+     constraints = NIL;
+     while (HeapTupleIsValid(constraintTuple = systable_getnext(scan))) {
+         Form_pg_constraint con = (Form_pg_constraint)(GETSTRUCT(constraintTuple));
+         bool isnull;
+         Datum consrc;
+         char *consrc_string;
+         if (con->contype != 'c')
+             continue;
+         consrc = fastgetattr(constraintTuple, Anum_pg_constraint_conbin, tupleDesc, &isnull);
+         if (isnull)
+             continue;
+         consrc = DirectFunctionCall2(pg_get_expr, consrc, ObjectIdGetDatum(con->conrelid));
+         consrc_string = DatumGetCString(DirectFunctionCall1(textout, consrc));
+         elog(DEBUG1, "Found child constraint \"%s\": \"%s\"", NameStr(con->conname), consrc_string);
+         constraints = lappend(constraints, consrc_string);
+     }
+     systable_endscan(scan);
+
+     ScanKeyInit(&key,
+                 Anum_pg_constraint_conrelid,
+                 BTEqualStrategyNumber, F_OIDEQ,
+                 ObjectIdGetDatum(RelationGetRelid(relation)));
+     scan = systable_beginscan(catalogRelation, ConstraintRelidIndexId, true, SnapshotNow, 1, &key);
+     while (HeapTupleIsValid(constraintTuple = systable_getnext(scan))) {
+         Form_pg_constraint con = (Form_pg_constraint)(GETSTRUCT(constraintTuple));
+         bool isnull;
+         Datum consrc;
+         char *consrc_string;
+         if (con->contype != 'c')
+             continue;
+         consrc = fastgetattr(constraintTuple, Anum_pg_constraint_conbin, tupleDesc, &isnull);
+         if (isnull)
+             continue;
+         consrc = DirectFunctionCall2(pg_get_expr, consrc, ObjectIdGetDatum(con->conrelid));
+         consrc_string = DatumGetCString(DirectFunctionCall1(textout, consrc));
+         elog(DEBUG1, "Looking for parent constraint \"%s\": \"%s\"", NameStr(con->conname), consrc_string);
+
+         if (!list_member_string(constraints, consrc_string))
+             ereport(ERROR,
+                     (errcode(ERRCODE_DATATYPE_MISMATCH),
+                      errmsg("child table missing constraint matching parent table constraint \"%s\"",
+                             NameStr(con->conname))));
+     }
+     systable_endscan(scan);
+
+     heap_close(catalogRelation, AccessShareLock);
+
+     catalogRelation = heap_open(InheritsRelationId, RowExclusiveLock);
+     StoreCatalogInheritance1(RelationGetRelid(rel), RelationGetRelid(relation), inhseqno+1, catalogRelation);
+     heap_close(catalogRelation, RowExclusiveLock);
+
+     heap_close(relation, AccessShareLock);
+ }
+
+ static void
+ MergeAttributesIntoExisting(Relation rel, Relation relation)
+ {
+     Relation     attrdesc;
+     AttrNumber    parent_attno, child_attno;
+     TupleDesc    tupleDesc;
+     TupleConstr *constr;
+     HeapTuple     tuple;
+
+     child_attno = RelationGetNumberOfAttributes(rel);
+
+     tupleDesc = RelationGetDescr(relation);
+     constr = tupleDesc->constr;
+
+     for (parent_attno = 1; parent_attno <= tupleDesc->natts;
+          parent_attno++)
+     {
+         Form_pg_attribute attribute = tupleDesc->attrs[parent_attno - 1];
+         char       *attributeName = NameStr(attribute->attname);
+
+         /* Ignore dropped columns in the parent. */
+         if (attribute->attisdropped)
+             continue;
+
+         /* Does it conflict with an existing column? */
+         attrdesc = heap_open(AttributeRelationId, RowExclusiveLock);
+
+         tuple = SearchSysCacheCopyAttName(RelationGetRelid(rel), attributeName);
+         if (HeapTupleIsValid(tuple)) {
+             /*
+              * Yes, try to merge the two column definitions. They must
+              * have the same type and typmod.
+              */
+             Form_pg_attribute childatt = (Form_pg_attribute) GETSTRUCT(tuple);
+             if (attribute->atttypid  != childatt->atttypid ||
+                 attribute->atttypmod != childatt->atttypmod ||
+                 (attribute->attnotnull && !childatt->attnotnull))
+                 ereport(ERROR,
+                         (errcode(ERRCODE_DATATYPE_MISMATCH),
+                          errmsg("child table \"%s\" has different type for column \"%s\"",
+                                 RelationGetRelationName(rel), NameStr(attribute->attname))));
+
+             childatt->attinhcount++;
+             simple_heap_update(attrdesc, &tuple->t_self, tuple);
+             CatalogUpdateIndexes(attrdesc, tuple); /* XXX strength reduce openindexes to outside loop? */
+             heap_freetuple(tuple);
+
+             /* We don't touch default at all since we're not making any other
+              * DDL changes to the child */
+
+         } else {
+             /*
+              * No, create a new inherited column
+              *
+              * Creating inherited columns in this case seems to be unpopular.
+              * In the common use case of partitioned tables it's a foot-gun.
+              */
+             ereport(ERROR,
+                     (errcode(ERRCODE_DATATYPE_MISMATCH),
+                      errmsg("child table missing column \"%s\"",
+                             NameStr(attribute->attname))));
+         }
+         heap_close(attrdesc, RowExclusiveLock);
+     }
+
+ }
+
+ static void
+ ATExecDropInherits(Relation rel, RangeVar *parent)
+ {
+
+
+     Relation    catalogRelation;
+     SysScanDesc scan;
+     ScanKeyData key[2];
+     HeapTuple    inheritsTuple, attributeTuple, depTuple;
+     Oid            inhparent;
+     Oid            dropparent;
+     int         found = 0;
+
+     /* Get the OID of parent -- if no schema is specified use the regular
+      * search path and only drop the one table that's found. We could try to be
+      * clever and look at each parent and see if it matches but that would be
+      * inconsistent with other operations I think. */
+
+     Assert(rel);
+     Assert(parent);
+
+     dropparent = RangeVarGetRelid(parent, false);
+
+     /* Search through the direct parents of rel looking for dropparent oid */
+
+     catalogRelation = heap_open(InheritsRelationId, RowExclusiveLock);
+     ScanKeyInit(key,
+                 Anum_pg_inherits_inhrelid,
+                 BTEqualStrategyNumber, F_OIDEQ,
+                 ObjectIdGetDatum(RelationGetRelid(rel)));
+     scan = systable_beginscan(catalogRelation, InheritsRelidSeqnoIndexId, true, SnapshotNow, 1, key);
+     while (!found && HeapTupleIsValid(inheritsTuple = systable_getnext(scan)))
+     {
+         inhparent = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhparent;
+         if (inhparent == dropparent) {
+             simple_heap_delete(catalogRelation, &inheritsTuple->t_self);
+             found = 1;
+         }
+     }
+     systable_endscan(scan);
+     heap_close(catalogRelation, RowExclusiveLock);
+
+
+     if (!found) {
+         /* would it be better to look up the actual schema of dropparent and
+          * make the error message explicitly name the qualified name it's
+          * trying to drop ?*/
+         if (parent->schemaname)
+             ereport(ERROR,
+                     (errcode(ERRCODE_UNDEFINED_TABLE),
+                      errmsg("relation \"%s.%s\" is not a parent of relation \"%s\"",
+                             parent->schemaname, parent->relname, RelationGetRelationName(rel))));
+         else
+             ereport(ERROR,
+                     (errcode(ERRCODE_UNDEFINED_TABLE),
+                      errmsg("relation \"%s\" is not a parent of relation \"%s\"",
+                             parent->relname, RelationGetRelationName(rel))));
+     }
+
+     /* Search through columns looking for matching columns from parent table */
+
+     catalogRelation = heap_open(AttributeRelationId, RowExclusiveLock);
+     ScanKeyInit(key,
+                 Anum_pg_attribute_attrelid,
+                 BTEqualStrategyNumber, F_OIDEQ,
+                 ObjectIdGetDatum(RelationGetRelid(rel)));
+     scan = systable_beginscan(catalogRelation, AttributeRelidNumIndexId, true, SnapshotNow, 1, key);
+     while (HeapTupleIsValid(attributeTuple = systable_getnext(scan))) {
+         Form_pg_attribute att = ((Form_pg_attribute)GETSTRUCT(attributeTuple));
+         /* Not an inherited column at all
+          * (do NOT use islocal for this test--it can be true for inherited columns)
+          */
+         if (att->attinhcount == 0)
+             continue;
+         if (att->attisdropped) /* XXX Is this right? */
+             continue;
+         if (SearchSysCacheExistsAttName(dropparent, NameStr(att->attname))) {
+             /* Decrement inhcount and possibly set islocal to 1 */
+             HeapTuple copyTuple = heap_copytuple(attributeTuple);
+             Form_pg_attribute copy_att = ((Form_pg_attribute)GETSTRUCT(copyTuple));
+
+             copy_att->attinhcount--;
+             if (copy_att->attinhcount == 0)
+                 copy_att->attislocal = 1;
+
+             simple_heap_update(catalogRelation, ©Tuple->t_self, copyTuple);
+             /* XXX "Avoid using it for multiple tuples, since opening the
+              * indexes and building the index info structures is moderately
+              * expensive." Perhaps this can be moved outside the loop or else
+              * at least the CatalogOpenIndexes/CatalogCloseIndexes moved
+              * outside the loop but when I try that it seg faults?!*/
+             CatalogUpdateIndexes(catalogRelation, copyTuple);
+             heap_freetuple(copyTuple);
+         }
+     }
+     systable_endscan(scan);
+     heap_close(catalogRelation, RowExclusiveLock);
+
+
+     /* Drop the dependency
+      *
+      * There's no convenient way to do this, so go trawling through pg_depend
+      */
+
+     catalogRelation = heap_open(DependRelationId, RowExclusiveLock);
+
+     ScanKeyInit(&key[0],
+                 Anum_pg_depend_classid,
+                 BTEqualStrategyNumber, F_OIDEQ,
+                 RelationRelationId);
+     ScanKeyInit(&key[1],
+                 Anum_pg_depend_objid,
+                 BTEqualStrategyNumber, F_OIDEQ,
+                 ObjectIdGetDatum(RelationGetRelid(rel)));
+
+     scan = systable_beginscan(catalogRelation, DependDependerIndexId, true,
+                               SnapshotNow, 2, key);
+
+     while (HeapTupleIsValid(depTuple = systable_getnext(scan))) {
+         Form_pg_depend dep = (Form_pg_depend) GETSTRUCT(depTuple);
+
+         if (dep->refclassid == RelationRelationId &&
+             dep->refobjid == dropparent &&
+             dep->deptype == DEPENDENCY_NORMAL) {
+
+             /* Only delete a single dependency -- there shouldn't be more but just in case... */
+             simple_heap_delete(catalogRelation, &depTuple->t_self);
+
+             break;
+         }
+     }
+     systable_endscan(scan);
+
+     heap_close(catalogRelation, RowExclusiveLock);
+ }
+
+
+
  /*
   * ALTER TABLE CREATE TOAST TABLE
   *
Index: src/backend/nodes/copyfuncs.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v
retrieving revision 1.335
diff -u -p -c -r1.335 copyfuncs.c
cvs diff: conflicting specifications of output style
*** src/backend/nodes/copyfuncs.c    30 Apr 2006 18:30:38 -0000    1.335
--- src/backend/nodes/copyfuncs.c    9 Jun 2006 21:28:14 -0000
*************** _copyAlterTableCmd(AlterTableCmd *from)
*** 1799,1804 ****
--- 1799,1805 ----
      COPY_SCALAR_FIELD(subtype);
      COPY_STRING_FIELD(name);
      COPY_NODE_FIELD(def);
+     COPY_NODE_FIELD(parent);
      COPY_NODE_FIELD(transform);
      COPY_SCALAR_FIELD(behavior);

Index: src/backend/parser/gram.y
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.545
diff -u -p -c -r2.545 gram.y
cvs diff: conflicting specifications of output style
*** src/backend/parser/gram.y    27 May 2006 17:38:45 -0000    2.545
--- src/backend/parser/gram.y    9 Jun 2006 21:28:15 -0000
*************** alter_table_cmd:
*** 1514,1519 ****
--- 1514,1535 ----
                      n->subtype = AT_DisableTrigUser;
                      $$ = (Node *)n;
                  }
+             /* ALTER TABLE <name> ALTER INHERITS ADD <parent> */
+             | INHERIT qualified_name
+                 {
+                     AlterTableCmd *n = makeNode(AlterTableCmd);
+                     n->subtype = AT_AddInherits;
+                     n->parent = $2;
+                     $$ = (Node *)n;
+                 }
+             /* ALTER TABLE <name> alter INHERITS DROP <parent> */
+             | NO INHERIT qualified_name
+                 {
+                     AlterTableCmd *n = makeNode(AlterTableCmd);
+                     n->subtype = AT_DropInherits;
+                     n->parent = $3;
+                     $$ = (Node *)n;
+                 }
              | alter_rel_cmd
                  {
                      $$ = $1;
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.310
diff -u -p -c -r1.310 parsenodes.h
cvs diff: conflicting specifications of output style
*** src/include/nodes/parsenodes.h    30 Apr 2006 18:30:40 -0000    1.310
--- src/include/nodes/parsenodes.h    9 Jun 2006 21:28:17 -0000
*************** typedef enum AlterTableType
*** 874,880 ****
      AT_EnableTrigAll,            /* ENABLE TRIGGER ALL */
      AT_DisableTrigAll,            /* DISABLE TRIGGER ALL */
      AT_EnableTrigUser,            /* ENABLE TRIGGER USER */
!     AT_DisableTrigUser            /* DISABLE TRIGGER USER */
  } AlterTableType;

  typedef struct AlterTableCmd    /* one subcommand of an ALTER TABLE */
--- 874,882 ----
      AT_EnableTrigAll,            /* ENABLE TRIGGER ALL */
      AT_DisableTrigAll,            /* DISABLE TRIGGER ALL */
      AT_EnableTrigUser,            /* ENABLE TRIGGER USER */
!     AT_DisableTrigUser,            /* DISABLE TRIGGER USER */
!     AT_AddInherits,                /* ADD INHERITS parent */
!     AT_DropInherits                /* DROP INHERITS parent */
  } AlterTableType;

  typedef struct AlterTableCmd    /* one subcommand of an ALTER TABLE */
*************** typedef struct AlterTableCmd    /* one subc
*** 883,888 ****
--- 885,891 ----
      AlterTableType subtype;        /* Type of table alteration to apply */
      char       *name;            /* column, constraint, or trigger to act on,
                                   * or new owner or tablespace */
+     RangeVar   *parent;            /* Parent table for add/drop inherits */
      Node       *def;            /* definition of new column, column type,
                                   * index, or constraint */
      Node       *transform;        /* transformation expr for ALTER TYPE */


--
greg

Re: ADD/DROP INHERITS

From
Tom Lane
Date:
Greg Stark <gsstark@mit.edu> writes:
>    I also haven't checked the constraint name. To do so it would make sense to
>    use a small hash table.

No, it'd make sense to use strcmp().  It's unlikely that there will be
enough constraints attached to any one table to justify use of any but
the simplest algorithm.  AFAICS you should just iterate through the
child constraints looking for matches ... and I'd suggest checking the
name first, as that will save a whole lot more work in reverse-compiling
than any amount of tenseness in the matching code.

>    I'm ignoring unique, primary key, and foreign key constraints on the theory
>    that these things don't really work on inherited tables yet
>    anyways.

Yeah, the consistent thing to do with these is nothing, until something
is done about the generic problem.

            regards, tom lane

Re: ADD/DROP INHERITS

From
Greg Stark
Date:
Tom Lane <tgl@sss.pgh.pa.us> writes:

> Greg Stark <gsstark@mit.edu> writes:
> >    I also haven't checked the constraint name. To do so it would make sense to
> >    use a small hash table.
>
> No, it'd make sense to use strcmp().  It's unlikely that there will be
> enough constraints attached to any one table to justify use of any but
> the simplest algorithm.  AFAICS you should just iterate through the
> child constraints looking for matches ... and I'd suggest checking the
> name first, as that will save a whole lot more work in reverse-compiling
> than any amount of tenseness in the matching code.

So should I set up a nested scan, essentially implementing a nested loop? or
should I gather together all the children in a list? My inclination is to
avoid the repeated scans and gather them together in a list of cons cells of
the two strings. Or can I stuff the whole tuple in the list elements? I'm
unclear on the memory management of tuples in the midst of a scan; would I
have to copy them?

Are the scans less expensive than I imagine and there's no point in storing
the results?

And are there any other fields of pg_constraint that I should be checking for
matches in? Do we care if a parent table has a non-deferrable constraint and
the child has a deferrable one, or if the parent's is deferred by default and
the child isn't?


> >    I'm ignoring unique, primary key, and foreign key constraints on the theory
> >    that these things don't really work on inherited tables yet
> >    anyways.
>
> Yeah, the consistent thing to do with these is nothing, until something
> is done about the generic problem.

It seems to me that foreign key constraints ought to be copied even now
though.

Also, it seems to me that LIKE ought to copy constraints or at least have an
option to. Otherwise it's not really suitable for creating partitions which
would be sad since it seems perfect for that task.

--
greg

Re: ADD/DROP INHERITS

From
Tom Lane
Date:
Greg Stark <gsstark@mit.edu> writes:
> So should I set up a nested scan, essentially implementing a nested loop? or
> should I gather together all the children in a list?

I'd use the predigested form of the constraints attached to the Relation
tupledescs, cf. RelationBuildTupleDesc, equalTupleDescs.  It might be
worth refactoring equalTupleDescs so you could share code --- ISTM what
you're trying to implement is something like a "subsetTupleDesc".

> And are there any other fields of pg_constraint that I should be checking for
> matches in? Do we care if a parent table has a non-deferrable constraint and
> the child has a deferrable one, or if the parent's is deferred by default and
> the child isn't?

I don't believe those attributes mean anything for check constraints
ATM, but you may as well compare them anyway.  If we ever do implement
them then it'd be reasonable to expect parent and child to have
identical settings.

> Also, it seems to me that LIKE ought to copy constraints or at least have an
> option to.

What does the spec say about that?

            regards, tom lane

Re: ADD/DROP INHERITS

From
Greg Stark
Date:
Tom Lane <tgl@sss.pgh.pa.us> writes:

> I don't believe those attributes mean anything for check constraints
> ATM, but you may as well compare them anyway.  If we ever do implement
> them then it'd be reasonable to expect parent and child to have
> identical settings.

I'm not sure. Does it have to have identical behaviour as long as it
guarantees the same level of data integrity? Deferred constraints will still
guarantee that the promises of the parent table are met.

But in that case I guess I really have to store the whole tuple. I'll look
into the stuff you suggested I look at to do that.

> > Also, it seems to me that LIKE ought to copy constraints or at least have an
> > option to.
>
> What does the spec say about that?

It says:

    NOTE 245 \u2014 <column constraint>s, except for NOT NULL, are not
    included in CDi; <column constraint definition>s are effectively
    transformed to <table constraint definition>s and are thereby also
    excluded.

We could still do an INCLUDING CONSTRAINTS option or something like that?

It seems it would make it much more convenient for creating partitions. Then
we could document that "CREATE TABLE child (LIKE parent INCLUDING
CONSTRAINTS)" is guaranteed to create a suitable child table for your parent
table.


--
greg

Re: ADD/DROP INHERITS

From
Greg Stark
Date:
Tom Lane <tgl@sss.pgh.pa.us> writes:

> Greg Stark <gsstark@mit.edu> writes:
> > So should I set up a nested scan, essentially implementing a nested loop? or
> > should I gather together all the children in a list?
>
> I'd use the predigested form of the constraints attached to the Relation
> tupledescs, cf. RelationBuildTupleDesc, equalTupleDescs.  It might be
> worth refactoring equalTupleDescs so you could share code --- ISTM what
> you're trying to implement is something like a "subsetTupleDesc".

Unless I'm missing something that predigested form only has the conbin field.
It doesn't have the name of the constraint nor the other fields like
deferrable and deferred by default. It also doesn't have foreign key
constraints which I'm ignoring now but suggesting that we will want to be
copying to children and checking for in new children in the future.

And subsetTupleDesc seems to be checking that the attributes are in the same
specific order, not that they match by name. That seems like a very different
kind of quality/subset nature than needed here.

--
greg

Re: ADD/DROP INHERITS

From
Greg Stark
Date:
Tom Lane <tgl@sss.pgh.pa.us> writes:

> Greg Stark <gsstark@mit.edu> writes:
> > So should I set up a nested scan, essentially implementing a nested loop? or
> > should I gather together all the children in a list?
>
> I'd use the predigested form of the constraints attached to the Relation
> tupledescs, cf. RelationBuildTupleDesc, equalTupleDescs.

I've spent more time absorbing relcache.c and I see now that there is in fact
a ccname array that contains the constraint names.

The relcache does not however contain the deferrable and isdeferred flags
which as you point out don't mean much for check constraints. But you said I
should be checking those. Going with the relcache would mean not being able to
check them.

Going with the relcache would also mean I wouldn't be able to easily add
foreign key constraints. I had hoped to add foreign key constraints to the
list of things that inherited tables copy and checking for them in
AddInherits.

So I'm thinking it's better to leave my implementation as is rather than
reimplement it using the relcache. Or would it be better to use the relcache
and then when and if it comes to making inherited tables inherit foreign key
constraints look into adding foreign keys and the deferrable and isdeffered
flags to the relcache?

> It might be worth refactoring equalTupleDescs so you could share code ---
> ISTM what you're trying to implement is something like a "subsetTupleDesc".

I still don't see how there's any refactoring possible here. I could move the
code into a function in tupdesc.c but then it would mean an entirely redundant
loop to bump attinhcount.

--
greg

Re: ADD/DROP INHERITS

From
Tom Lane
Date:
Greg Stark <gsstark@mit.edu> writes:
> So I'm thinking it's better to leave my implementation as is rather than
> reimplement it using the relcache. Or would it be better to use the relcache
> and then when and if it comes to making inherited tables inherit foreign key
> constraints look into adding foreign keys and the deferrable and isdeffered
> flags to the relcache?

Well, it's reasonable to assume that the relcache entries for check
constraints include everything that's semantically meaningful, because
that's what the actual constraint enforcement code looks at.  IE, if
we ever did have deferrable check constraints then that flag would get
added to the entries.

However, we don't keep any info about FK constraints in the relcache
(except indirectly via their triggers).  At the moment I can't think
of any reason why we should.

If you're happy with the code looking directly at pg_constraint then
I see no reason to change it.  I just mentioned the relcache because
I thought you were concerned about the performance of a pg_constraint
search.

            regards, tom lane

Re: ADD/DROP INHERITS

From
Greg Stark
Date:
Tom Lane <tgl@sss.pgh.pa.us> writes:

> If you're happy with the code looking directly at pg_constraint then
> I see no reason to change it.  I just mentioned the relcache because
> I thought you were concerned about the performance of a pg_constraint
> search.

I'm not concerned with the performance hit of doing a linear scan on
pg_constraint or pg_attribute.

I am slightly concerned about repeatedly calling SearchSysCacheExistsAttName
But using relcache would mean a O(n^2) search across the attributes which
might be even worse. I'm unclear how efficient the SysCache lookup function
is. If it's a hash table lookup it might be slower but more scalable than an
O(n^2) match against the relcache anyways.

And I'm slightly concerned with the O(n^2) constraint matching. If someone has
100+ constraints it may be somewhat disappointing to have the operation have a
noticeable delay. 1,000 constraints means a million calls to strcmp.

Realistically though 1,000 check constraints would be pretty unlikely. 100
constraints might be on the edge of reasonableness and 10,000 calls to strcmp
is probably also at the edge of reasonableness too.

--
greg