DROP CONSTRAINT patch
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
Attachments:
dropcons.difftext/plain; charset=US-ASCII; name=dropcons.diffDownload
? 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 */
Anyone looked at this yet?
Also, if someone could tell me where I should attempt to add a regression
test and what, exactly, I should be regression testing it would be
helpful...
Chris
Show quoted text
-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org]On Behalf Of Christopher
Sent: Sunday, 20 May 2001 3:50 PM
To: pgsql-hackers@postgresql.org
Subject: [HACKERS] DROP CONSTRAINT patchHi 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
Anyone looked at this yet?
Also, if someone could tell me where I should attempt to add a regression
test and what, exactly, I should be regression testing it would be
helpful...
I was hoping someone else would comment on it. I will check it out
later and either apply or comment. You seemed little uncertain of it so
I left it in my mailbox for a while.
--
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 19026No actually, I'm quite confident of it - it works perfectly on my computer
and does exactly what it's supposed to do. I have tested it extensively.
It also works in a transaction and gets rolled back properly. However, I
haven't tested it with multiple backends simultaneously doing stuff. I'm
pretty sure there won't be anything wrong with it though...
It's the stylistic stuff I don't know about. For example, I issue a NOTICE
if more than one constraint was removed - I don't know if this is desirable
behaviour from the viewpoint of you guys (I personally think it's very
important!).
But hey...
Chris
Show quoted text
-----Original Message-----
From: Bruce Momjian [mailto:pgman@candle.pha.pa.us]
Sent: Tuesday, 22 May 2001 7:13 PM
To: Christopher Kings-Lynne
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] DROP CONSTRAINT patchAnyone looked at this yet?
Also, if someone could tell me where I should attempt to add a
regression
test and what, exactly, I should be regression testing it would be
helpful...I was hoping someone else would comment on it. I will check it out
later and either apply or comment. You seemed little uncertain of it so
I left it in my mailbox for a while.-- 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
On Tue, 22 May 2001, Christopher Kings-Lynne wrote:
Anyone looked at this yet?
Also, if someone could tell me where I should attempt to add a regression
test and what, exactly, I should be regression testing it would be
helpful...
At the risk of making it even longer, probably alter_table.sql.
You probably want to try out various conceivable uses of the drop
constraint, including error conditions.
Some things like:
create table with constraint
try to insert valid row
try to insert invalid row
drop the constraint
try to insert valid row
try to insert row that was invalid
create table with two equal named constraints
insert valid to both
insert valid to one but not two
insert valid to two but not one
insert valid to neither
...
create table with two non-equal named constraints
(do inserts)
drop constraint one
try to insert valid for both,
valid for one but not two
valid for two but not one
valid for neither
drop constraint two
(do more inserts)
For the add/drop constraint clauses would it be an idea to change the syntax
to:
ALTER TABLE [ ONLY ] x ADD CONSTRAINT x;
ALTER TABLE [ ONLY ] x DROP CONSTRAINT x;
So that people can specify whether the constraint should be inherited or
not?
Chris
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it withing the next 48 hours.
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?
--
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 19026On Wed, 23 May 2001, Christopher Kings-Lynne wrote:
For the add/drop constraint clauses would it be an idea to change the syntax
to:ALTER TABLE [ ONLY ] x ADD CONSTRAINT x;
ALTER TABLE [ ONLY ] x DROP CONSTRAINT x;So that people can specify whether the constraint should be inherited or
not?
I'm not sure. I don't much use the inheritance stuff, so I'm not sure
what would be better for them. Hopefully one of the people interested
in inheritance will speak up. :) Technically it's probably not difficult
to do.
A related question is whether or not you can drop a constraint on a
subtable that's inherited from a parent.
Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
On Wed, 23 May 2001, Christopher Kings-Lynne wrote:
For the add/drop constraint clauses would it be an idea to change the syntax
to:ALTER TABLE [ ONLY ] x ADD CONSTRAINT x;
ALTER TABLE [ ONLY ] x DROP CONSTRAINT x;
If the patch is coded in the same style as the existing ALTER code then
this will happen automatically. Are you looking at current development
tip as the comparison point for your changes?
A related question is whether or not you can drop a constraint on a
subtable that's inherited from a parent.
There is the question of whether it's a good idea to allow a constraint
to exist on a parent but not on its subtables. Seems like a bad idea to
me. But as long as the default is to propagate these changes, I'm not
really eager to prohibit DBAs from doing the other. Who's to say what's
a misuse of inheritance and what's not...
regards, tom lane
Your patch has been added to the PostgreSQL unapplied patches list at:
Would you like me to submit a version minus all my questions? And what
about the inheritance feature - should it be left enabled (as is) at the
moment?
Chris
Stephan Szabo <sszabo@megazone23.bigpanda.com> writes:
On Wed, 23 May 2001, Christopher Kings-Lynne wrote:
For the add/drop constraint clauses would it be an idea to
change the syntax
to:
ALTER TABLE [ ONLY ] x ADD CONSTRAINT x;
ALTER TABLE [ ONLY ] x DROP CONSTRAINT x;If the patch is coded in the same style as the existing ALTER code then
this will happen automatically. Are you looking at current development
tip as the comparison point for your changes?
I'm not sure what you mean here, Tom - I meant that the ONLY keyword could
be optional. I know that most of the existing ADD CONSTRAINT code does not
propagate constraints to children. However, if it was ever changed so that
it did - would it be nice to allow the DBA to specify that it should not be
propagated.
A related question is whether or not you can drop a constraint on a
subtable that's inherited from a parent.There is the question of whether it's a good idea to allow a constraint
to exist on a parent but not on its subtables.
It seems to me that someone needs to sit down and decide on the inheritance
semantics that should be enforced (ideally) and then they can be coded to
the design.
Seems like a bad idea to
me. But as long as the default is to propagate these changes, I'm not
really eager to prohibit DBAs from doing the other. Who's to say what's
a misuse of inheritance and what's not...
At the moment we have:
* ADD CONSTRAINT does not propagate
* If you create a table with a CHECK constraint, then create a table that
inherits from that, the CHECK constraint _does_ propagate.
Seems to me that these behaviours are inconsistent...
Chris
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
I'm not sure what you mean here, Tom - I meant that the ONLY keyword could
be optional.
The current gram.y code allows either ALTER TABLE foo ONLY or ALTER
TABLE foo* for all forms of ALTER ... with the default interpretation
being the latter.
At the moment we have:
* ADD CONSTRAINT does not propagate
I doubt you will find anyone who's willing to argue that that's not a
bug --- specifically, AlterTableAddConstraint()'s lack of inheritance
recursion like its siblings have. Feel free to fix it.
regards, tom lane
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
I'm not sure what you mean here, Tom - I meant that the ONLY
keyword could
be optional.
The current gram.y code allows either ALTER TABLE foo ONLY or ALTER
TABLE foo* for all forms of ALTER ... with the default interpretation
being the latter.
Oops - ok, I didn't notice that...hmmm...maybe I should check my patch for
that before Bruce commits it...
At the moment we have:
* ADD CONSTRAINT does not propagateI doubt you will find anyone who's willing to argue that that's not a
bug --- specifically, AlterTableAddConstraint()'s lack of inheritance
recursion like its siblings have. Feel free to fix it.
It was next on my list. However, I don't want to step on Stephan's toes...
Chris
[ Charset ISO-8859-1 unsupported, converting... ]
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes:
I'm not sure what you mean here, Tom - I meant that the ONLY
keyword could
be optional.
The current gram.y code allows either ALTER TABLE foo ONLY or ALTER
TABLE foo* for all forms of ALTER ... with the default interpretation
being the latter.Oops - ok, I didn't notice that...hmmm...maybe I should check my patch for
that before Bruce commits it...
Got it. Patch on hold.
As you can see, inheritance really needs some attention. Feel free to
give it.
--
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 19026Your patch has been added to the PostgreSQL unapplied patches list at:
Would you like me to submit a version minus all my questions? And what
about the inheritance feature - should it be left enabled (as is) at the
moment?
You can send me a totally new patch if you wish. That way, you can
throw the whole patch to the patches list for comment and improvement.
If you would prefer for me to apply it in pieces, I can.
Please keep going and resolve these inheritance issues.
--
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 19026You can send me a totally new patch if you wish. That way, you can
throw the whole patch to the patches list for comment and improvement.
I'll do that.
Please keep going and resolve these inheritance issues.
It's not that there's inheritance issues in my patch. I am actually certain
that it will work fine with the ONLY keyword. However, I will make some
changes and submit it to pgsql-patches.
The problem is that although my patch can quite happily remove all
constraints of the same name from all the child tables, the problem is - can
we be sure that all the constraints with that name are actually inherited
from the parent constraint. Answer - no. At least, not until the add
constraint stuff is all fixed. So maybe I will just hash out inheritance
support for the time being.
Chris
You can send me a totally new patch if you wish. That way, you can
throw the whole patch to the patches list for comment and improvement.I'll do that.
Please keep going and resolve these inheritance issues.
It's not that there's inheritance issues in my patch. I am actually certain
that it will work fine with the ONLY keyword. However, I will make some
changes and submit it to pgsql-patches.The problem is that although my patch can quite happily remove all
constraints of the same name from all the child tables, the problem is - can
we be sure that all the constraints with that name are actually inherited
from the parent constraint. Answer - no. At least, not until the add
constraint stuff is all fixed. So maybe I will just hash out inheritance
support for the time being.
Oh, I was hoping you could at least list _all_ the inheritance issues
for the TODO list. Seems you have found some already. If we have that,
we can get a plan for someone to finally fix them all.
What we really need is a list, and a discussion, and decisions, rather
than just letting it lay around.
--
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 19026Seems like a bad idea to
me. But as long as the default is to propagate these changes, I'm not
really eager to prohibit DBAs from doing the other. Who's to say what's
a misuse of inheritance and what's not...At the moment we have:
* ADD CONSTRAINT does not propagate
* If you create a table with a CHECK constraint, then create a table that
inherits from that, the CHECK constraint _does_ propagate.Seems to me that these behaviours are inconsistent...
Yep, but I've got the minimal patch to fix ADD CONSTRAINT. I'm just
waiting for the upcoming weekend so I can add the regression tests and
pack it up.
Thanks. Patch applied. @@ comments removed because patch was reviewed.
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?
--
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 19026Thanks. 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?-- 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 19026Attachments:
/bjm/2text/plainDownload
? 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 */