Re: DROP CONSTRAINT patch - Mailing list pgsql-hackers

>
> Thanks.  Patch applied.  @@ comments removed because patch was reviewed.
>

Actually, can someone make sure all the @@ comments have been dealt
with?  I assume people checked them, but maybe not.


> > Hi all,
> >
> > Attached is my patch that adds DROP CONSTRAINT support to PostgreSQL.  I
> > basically want your guys feedback.  I have sprinkled some of my q's thru
> > the text delimited with the @@ symbol.  It seems to work perfectly.
> >
> > At the moment it does CHECK constraints only, with inheritance.  However,
> > due to the problem mentioned before with the mismatching between inherited
> > constraints it may be wise to disable the inheritance feature for a while.
> > it is written in an extensible fashion to support future dropping of other
> > types of constraint, and is well documented.
> >
> > Please send me your comments, check my use of locking, updating of
> > indices, use of ERROR and NOTICE, etc. and I will rework the patch based
> > on feedback until everyone
> > is happy with it...
> >
> > Chris
>
> Content-Description:
>
> [ Attachment, skipping... ]
>
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 6: Have you searched our list archives?
> >
> > http://www.postgresql.org/search.mpl
>
> --
>   Bruce Momjian                        |  http://candle.pha.pa.us
>   pgman@candle.pha.pa.us               |  (610) 853-3000
>   +  If your life is a hard drive,     |  830 Blythe Avenue
>   +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
? config.log
? config.cache
? config.status
? dropcons.diff
? GNUmakefile
? src/GNUmakefile
? src/Makefile.global
? src/backend/postgres
? src/backend/catalog/global.bki
? src/backend/catalog/global.description
? src/backend/catalog/template1.bki
? src/backend/catalog/template1.description
? src/backend/port/Makefile
? src/bin/initdb/initdb
? src/bin/initlocation/initlocation
? src/bin/ipcclean/ipcclean
? src/bin/pg_config/pg_config
? src/bin/pg_ctl/pg_ctl
? src/bin/pg_dump/pg_dump
? src/bin/pg_dump/pg_restore
? src/bin/pg_dump/pg_dumpall
? src/bin/pg_id/pg_id
? src/bin/pg_passwd/pg_passwd
? src/bin/psql/psql
? src/bin/scripts/createlang
? src/include/config.h
? src/include/stamp-h
? src/interfaces/ecpg/lib/libecpg.so.3
? src/interfaces/ecpg/preproc/ecpg
? src/interfaces/libpgeasy/libpgeasy.so.2
? src/interfaces/libpq/libpq.so.2
? src/pl/plpgsql/src/libplpgsql.so.1
? src/test/regress/log
? src/test/regress/pg_regress
? src/test/regress/tmp_check
? src/test/regress/results
? src/test/regress/expected/copy.out
? src/test/regress/expected/create_function_1.out
? src/test/regress/expected/create_function_2.out
? src/test/regress/expected/misc.out
? src/test/regress/expected/constraints.out
? src/test/regress/sql/copy.sql
? src/test/regress/sql/create_function_1.sql
? src/test/regress/sql/create_function_2.sql
? src/test/regress/sql/misc.sql
? src/test/regress/sql/constraints.sql
Index: src/backend/catalog/heap.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/heap.c,v
retrieving revision 1.165
diff -c -r1.165 heap.c
*** src/backend/catalog/heap.c    2001/05/14 20:30:19    1.165
--- src/backend/catalog/heap.c    2001/05/20 07:27:27
***************
*** 48,53 ****
--- 48,54 ----
  #include "miscadmin.h"
  #include "optimizer/clauses.h"
  #include "optimizer/planmain.h"
+ #include "optimizer/prep.h"
  #include "optimizer/var.h"
  #include "nodes/makefuncs.h"
  #include "parser/parse_clause.h"
***************
*** 1975,1980 ****
--- 1976,2138 ----

      heap_endscan(rcscan);
      heap_close(rcrel, RowExclusiveLock);
+
+ }
+
+ /*
+  * Removes all CHECK constraints on a relation that match the given name.
+  * It is the responsibility of the calling function to acquire a lock on
+  * the relation.
+  * Returns: The number of CHECK constraints removed.
+  */
+ int
+ RemoveCheckConstraint(Relation rel, const char *constrName, bool inh)
+ {
+    Oid            relid;
+     Relation            rcrel;
+     Relation            relrel;
+     Relation            inhrel;
+     Relation            relidescs[Num_pg_class_indices];
+     TupleDesc        tupleDesc;
+     TupleConstr        *oldconstr;
+     int                numoldchecks;
+     int                numchecks;
+     HeapScanDesc    rcscan;
+     ScanKeyData        key[2];
+     HeapTuple        rctup;
+     HeapTuple        reltup;
+     Form_pg_class    relStruct;
+     int                rel_deleted = 0;
+    int            all_deleted = 0;
+
+    /* Find id of the relation */
+    /* @@ Does this need to be done _after_ the heap_open
+       has acquired a lock? @@ */
+    relid = RelationGetRelid(rel);
+
+    /* Process child tables and remove constraints of the
+       same name. */
+    if (inh)
+    {
+       List  *child,
+             *children;
+
+       /* This routine is actually in the planner */
+       children = find_all_inheritors(relid);
+
+       /*
+        * find_all_inheritors does the recursive search of the
+        * inheritance hierarchy, so all we have to do is process all
+        * of the relids in the list that it returns.
+        */
+       foreach(child, children)
+       {
+          Oid    childrelid = lfirsti(child);
+
+          if (childrelid == relid)
+             continue;
+          inhrel = heap_open(childrelid, AccessExclusiveLock);
+          all_deleted += RemoveCheckConstraint(inhrel, constrName, false);
+          heap_close(inhrel, NoLock);
+       }
+    }
+
+     /* Grab an exclusive lock on the pg_relcheck relation */
+     rcrel = heap_openr(RelCheckRelationName, RowExclusiveLock);
+
+     /*
+      * Create two scan keys.  We need to match on the oid of the table
+      * the CHECK is in and also we need to match the name of the CHECK
+      * constraint.
+      */
+     ScanKeyEntryInitialize(&key[0], 0, Anum_pg_relcheck_rcrelid,
+                            F_OIDEQ, RelationGetRelid(rel));
+
+     /* @@ F_NAMEEQ correct? @@ */
+     ScanKeyEntryInitialize(&key[1], 0, Anum_pg_relcheck_rcname,
+                            F_NAMEEQ, PointerGetDatum(constrName));
+
+     /* @@ &key needed here? @@ */
+     /* Begin scanning the heap */
+     rcscan = heap_beginscan(rcrel, 0, SnapshotNow, 2, key);
+
+     /*
+      * Scan over the result set, removing any matching entries.  Note
+      * that this has the side-effect of removing ALL CHECK constraints
+      * that share the specified constraint name.
+      */
+     while (HeapTupleIsValid(rctup = heap_getnext(rcscan, 0))) {
+         simple_heap_delete(rcrel, &rctup->t_self);
+         ++rel_deleted;
+       ++all_deleted;
+     }
+
+     /* Clean up after the scan */
+     heap_endscan(rcscan);
+
+     /* @@ Do I need to heap_close here? @@ */
+
+     /*
+      * @@ I copied this from elsewhere - is it still appropriate? @@
+      * Update the count of constraints in the relation's pg_class tuple.
+      * We do this even if there was no change, in order to ensure that an
+      * SI update message is sent out for the pg_class tuple, which will
+      * force other backends to rebuild their relcache entries for the rel.
+      * (Of course, for a newly created rel there is no need for an SI
+      * message, but for ALTER TABLE ADD ATTRIBUTE this'd be important.)
+      */
+
+      /*
+      * Get number of existing constraints.
+      * @@ Does this need to be above where we delete them!? It seems
+     * to work like it is! @@
+      */
+
+     tupleDesc = RelationGetDescr(rel);
+     oldconstr = tupleDesc->constr;
+     if (oldconstr)
+         numoldchecks = oldconstr->num_check;
+     else
+         numoldchecks = 0;
+     /* @@ Do we need to pfree oldconstr? @@ */
+
+     /* Calculate the new number of checks in the table, fail if negative */
+     numchecks = numoldchecks - rel_deleted;
+
+     if (numchecks < 0)
+         elog(ERROR, "check count became negative");
+
+     /* @@ Should I check if rel_deleted > 0 here for
+       efficiency? (re: SI Updates)@@ */
+     /* @@ Do I need to heap_open here? @@ */
+     relrel = heap_openr(RelationRelationName, RowExclusiveLock);
+     reltup = SearchSysCacheCopy(RELOID,
+         ObjectIdGetDatum(RelationGetRelid(rel)), 0, 0, 0);
+
+     if (!HeapTupleIsValid(reltup))
+         elog(ERROR, "cache lookup of relation %u failed",
+              RelationGetRelid(rel));
+     relStruct = (Form_pg_class) GETSTRUCT(reltup);
+
+     relStruct->relchecks = numchecks;
+
+     simple_heap_update(relrel, &reltup->t_self, reltup);
+
+     /* Keep catalog indices current */
+     CatalogOpenIndices(Num_pg_class_indices, Name_pg_class_indices,
+                        relidescs);
+     CatalogIndexInsert(relidescs, Num_pg_class_indices, relrel, reltup);
+     CatalogCloseIndices(Num_pg_class_indices, relidescs);
+
+     /* Clean up after the scan */
+     heap_freetuple(reltup);
+     heap_close(relrel, RowExclusiveLock);
+
+     /* Close the heap relation */
+     heap_close(rcrel, RowExclusiveLock);
+
+     /* Return the number of tuples deleted */
+     return all_deleted;
  }

  static void
Index: src/backend/commands/command.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/commands/command.c,v
retrieving revision 1.127
diff -c -r1.127 command.c
*** src/backend/commands/command.c    2001/05/09 21:10:38    1.127
--- src/backend/commands/command.c    2001/05/20 07:27:32
***************
*** 51,59 ****
  #include "catalog/pg_shadow.h"
  #include "utils/relcache.h"

- #ifdef    _DROP_COLUMN_HACK__
  #include "parser/parse.h"
- #endif     /* _DROP_COLUMN_HACK__ */
  #include "access/genam.h"


--- 51,57 ----
***************
*** 1322,1327 ****
--- 1320,1330 ----

                              break;
                          }
+                     case CONSTR_PRIMARY:
+                         {
+
+                             break;
+                         }
                      default:
                          elog(ERROR, "ALTER TABLE / ADD CONSTRAINT is not implemented for that constraint type.");
                  }
***************
*** 1583,1595 ****

  /*
   * ALTER TABLE DROP CONSTRAINT
   */
  void
  AlterTableDropConstraint(const char *relationName,
                           bool inh, const char *constrName,
                           int behavior)
  {
!     elog(ERROR, "ALTER TABLE / DROP CONSTRAINT is not implemented");
  }


--- 1586,1657 ----

  /*
   * ALTER TABLE DROP CONSTRAINT
+  * Note: It is legal to remove a constraint with name "" as it is possible
+  * to add a constraint with name "".
+  * Christopher Kings-Lynne
   */
  void
  AlterTableDropConstraint(const char *relationName,
                           bool inh, const char *constrName,
                           int behavior)
  {
!     Relation        rel;
!     int            deleted;
!
! #ifndef NO_SECURITY
!     if (!pg_ownercheck(GetUserId(), relationName, RELNAME))
!         elog(ERROR, "ALTER TABLE: permission denied");
! #endif
!
!     /* We don't support CASCADE yet  - in fact, RESTRICT
!      * doesn't work to the spec either! */
!     if (behavior == CASCADE)
!         elog(ERROR, "ALTER TABLE / DROP CONSTRAINT does not support the CASCADE keyword");
!
!     /*
!      * Acquire an exclusive lock on the target relation for
!      * the duration of the operation.
!      * @@ AccessExclusiveLock or RowExclusiveLock??? @@
!      */
!
!     rel = heap_openr(relationName, AccessExclusiveLock);
!
!     /* Disallow DROP CONSTRAINT on views, indexes, sequences, etc */
!     if (rel->rd_rel->relkind != RELKIND_RELATION)
!         elog(ERROR, "ALTER TABLE / DROP CONSTRAINT: %s is not a table",
!              relationName);
!
!     /*
!      * Since all we have is the name of the constraint, we have to look through
!      * all catalogs that could possibly contain a constraint for this relation.
!      * We also keep a count of the number of constraints removed.
!      */
!
!     deleted = 0;
!
!     /*
!      * First, we remove all CHECK constraints with the given name
!      */
!
!     deleted += RemoveCheckConstraint(rel, constrName, inh);
!
!     /*
!      * Now we remove NULL, UNIQUE, PRIMARY KEY and FOREIGN KEY constraints.
!      *
!      * Unimplemented.
!      */
!
!     /* Close the target relation */
!     heap_close(rel, NoLock);
!
!     /* If zero constraints deleted, complain */
!     if (deleted == 0)
!         elog(ERROR, "ALTER TABLE / DROP CONSTRAINT: %s does not exist",
!              constrName);
!     /* Otherwise if more than one constraint deleted, notify */
!     else if (deleted > 1)
!         elog(NOTICE, "Multiple constraints dropped");
!
  }


Index: src/include/catalog/heap.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/catalog/heap.h,v
retrieving revision 1.35
diff -c -r1.35 heap.h
*** src/include/catalog/heap.h    2001/05/07 00:43:24    1.35
--- src/include/catalog/heap.h    2001/05/20 07:27:38
***************
*** 45,50 ****
--- 45,53 ----
                            List *rawColDefaults,
                            List *rawConstraints);

+ extern int RemoveCheckConstraint(Relation rel, const char *constrName, bool inh);
+
+
  extern Form_pg_attribute SystemAttributeDefinition(AttrNumber attno);

  #endif     /* HEAP_H */