Thread: Patch for ALTER TABLE / TYPE
Hi, Please find the patch attached. This is for the bug which is posted to hackers before. http://archives.postgresql.org/pgsql-hackers/2005-06/msg01442.php We can see a problem by this bug in following way. CREATE TABLE pktable (a int primary key); CREATE TABLE fktable (b int references pktable); ALTER TABLE pktable ALTER COLUMN a TYPE bigint; -- succeed REINDEX TABLE pg_depend; ALTER TABLE pktable ALTER COLUMN a TYPE int; -- fail NOTICE: constraint fktable_b_fkey on table fktable depends on index pktable_pkey ERROR: cannot drop constraint pktable_pkey on table pktable because other objects depend on it HINT: Use DROP ... CASCADE to drop the dependent objects too. I changed the order of constraints list to delete foreign key constraints first. Any comments are welcome. Regards, Nakano *** postgresql-8.1.2-org/src/backend/commands/tablecmds.c 2005-11-23 03:23:07.000000000 +0900 --- postgresql-8.1.2/src/backend/commands/tablecmds.c 2006-01-27 21:46:54.000000000 +0900 *************** *** 4970,4979 **** Assert(foundObject.objectSubId == 0); if (!list_member_oid(tab->changedConstraintOids, foundObject.objectId)) { ! tab->changedConstraintOids = lappend_oid(tab->changedConstraintOids, ! foundObject.objectId); ! tab->changedConstraintDefs = lappend(tab->changedConstraintDefs, ! pg_get_constraintdef_string(foundObject.objectId)); } break; --- 4970,5022 ---- Assert(foundObject.objectSubId == 0); if (!list_member_oid(tab->changedConstraintOids, foundObject.objectId)) { ! Oid conOid = foundObject.objectId; ! Relation conRel; ! SysScanDesc conScan; ! ScanKeyData conKey[1]; ! HeapTuple conTup; ! Form_pg_constraint conForm; ! char *defstring = pg_get_constraintdef_string(foundObject.objectId); ! ! /* Look up the target constraint */ ! conRel = heap_open(ConstraintRelationId, AccessShareLock); ! ! ScanKeyInit(&conKey[0], ! ObjectIdAttributeNumber, ! BTEqualStrategyNumber, F_OIDEQ, ! ObjectIdGetDatum(conOid)); ! ! conScan = systable_beginscan(conRel, ConstraintOidIndexId, true, ! SnapshotNow, 1, conKey); ! ! /* This should not fail, since pg_get_constraint_string found it. */ ! conTup = systable_getnext(conScan); ! ! conForm = (Form_pg_constraint) GETSTRUCT(conTup); ! ! /* ! * FOREIGN KEY constraints depend on the indexes which are depend ! * on PRIMARY KEY constraints, so we need to append FOREIGN KEY ! * constraints ahead of PRIMARY KEY constraints. Otherwise deletion ! * of a PRIMARY KEY constraint with RESTRICT in cleanup will fail. ! */ ! if (conForm->contype == CONSTRAINT_FOREIGN) ! { ! tab->changedConstraintOids = lcons_oid(conOid, ! tab->changedConstraintOids); ! tab->changedConstraintDefs = lcons(defstring, ! tab->changedConstraintDefs); ! } ! else ! { ! tab->changedConstraintOids = lappend_oid(tab->changedConstraintOids, ! conOid); ! tab->changedConstraintDefs = lappend(tab->changedConstraintDefs, ! defstring); ! } ! ! systable_endscan(conScan); ! heap_close(conRel, AccessShareLock); } break; *************** *** 5141,5149 **** /* * Now we can drop the existing constraints and indexes --- constraints ! * first, since some of them might depend on the indexes. It should be ! * okay to use DROP_RESTRICT here, since nothing else should be depending ! * on these objects. */ foreach(l, tab->changedConstraintOids) { --- 5184,5193 ---- /* * Now we can drop the existing constraints and indexes --- constraints ! * first, since some of them might depend on the indexes. (FOREIGN KEY ! * constraints depend on the indexes, but these are deleted first.) ! * It should be okay to use DROP_RESTRICT here, since nothing else should ! * be depending on these objects. */ foreach(l, tab->changedConstraintOids) {
NAKANO Yoshihisa <nakano.yosihisa@jp.fujitsu.com> writes: > Please find the patch attached. This is for the bug which is posted to > hackers before. > http://archives.postgresql.org/pgsql-hackers/2005-06/msg01442.php > We can see a problem by this bug in following way. > CREATE TABLE pktable (a int primary key); > CREATE TABLE fktable (b int references pktable); > ALTER TABLE pktable ALTER COLUMN a TYPE bigint; -- succeed > REINDEX TABLE pg_depend; > ALTER TABLE pktable ALTER COLUMN a TYPE int; -- fail > NOTICE: constraint fktable_b_fkey on table fktable depends on index > pktable_pkey > ERROR: cannot drop constraint pktable_pkey on table pktable because > other objects depend on it > HINT: Use DROP ... CASCADE to drop the dependent objects too. Ah, thanks for providing the simple test case. Much nicer than Neil's way... I don't much like the patch though :-(. It seems like a brute force solution, and it's lacking error checking. After looking at the test case a bit, I have an alternate approach: constraints on the column will have DEPENDENCY_AUTO type, while constraints using the column will have DEPENDENCY_NORMAL type. Therefore, if we drop the NORMAL ones before the AUTO ones, that should be enough to fix it. This doesn't require much extra code, or any extra catalog searches, since the pg_depend record is already available in ATExecAlterColumnType where we need to decide whether to stick the item on the front or back of the list. regards, tom lane
Tom Lane wrote:
> After looking at the test case a bit, I have an alternate approach:
> constraints on the column will have DEPENDENCY_AUTO type, while
> constraints using the column will have DEPENDENCY_NORMAL type.
> Therefore, if we drop the NORMAL ones before the AUTO ones, that
> should be enough to fix it. This doesn't require much extra code,
> or any extra catalog searches, since the pg_depend record is already
> available in ATExecAlterColumnType where we need to decide whether
> to stick the item on the front or back of the list.
O.K. It seems nicer than my solution.
Please find the patch attached. I fixed the patch to decide the order
of the list by deptype of the pg_depend record.
Regards,
Nakano
*** postgresql-8.1.2-org/src/backend/commands/tablecmds.c 2005-11-23 03:23:07.000000000 +0900
--- postgresql-8.1.2/src/backend/commands/tablecmds.c 2006-01-30 09:43:13.000000000 +0900
***************
*** 4970,4979 ****
Assert(foundObject.objectSubId == 0);
if (!list_member_oid(tab->changedConstraintOids, foundObject.objectId))
{
! tab->changedConstraintOids = lappend_oid(tab->changedConstraintOids,
! foundObject.objectId);
! tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
! pg_get_constraintdef_string(foundObject.objectId));
}
break;
--- 4970,4996 ----
Assert(foundObject.objectSubId == 0);
if (!list_member_oid(tab->changedConstraintOids, foundObject.objectId))
{
! char *defstring = pg_get_constraintdef_string(foundObject.objectId);
!
! /* FOREIGN KEY constraints depend on the indexes which depend on
! * PRIMARY KEY constraints, so we need to append FOREIGN KEY
! * constraints ahead of PRIMARY KEY constraints. We decide this
! * by the dependency type.
! */
! if (foundDep->deptype == DEPENDENCY_NORMAL)
! {
! tab->changedConstraintOids = lcons_oid(foundObject.objectId,
! tab->changedConstraintOids);
! tab->changedConstraintDefs = lcons(defstring,
! tab->changedConstraintDefs);
! }
! else
! {
! tab->changedConstraintOids = lappend_oid(tab->changedConstraintOids,
! foundObject.objectId);
! tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
! defstring);
! }
}
break;
***************
*** 5141,5149 ****
/*
* Now we can drop the existing constraints and indexes --- constraints
! * first, since some of them might depend on the indexes. It should be
! * okay to use DROP_RESTRICT here, since nothing else should be depending
! * on these objects.
*/
foreach(l, tab->changedConstraintOids)
{
--- 5158,5168 ----
/*
* Now we can drop the existing constraints and indexes --- constraints
! * first, since some of them might depend on the indexes. (FOREIGN KEY
! * constraints depend on the indexes of PRIMARY KEY constraints, but
! * these will have DEPENDENCY_NORMAL type and will be deleted before
! * PRIMARY KEY constraints.) It should be okay to use DROP_RESTRICT here,
! * since nothing else should be depending on these objects.
*/
foreach(l, tab->changedConstraintOids)
{
NAKANO Yoshihisa <nakano.yosihisa@jp.fujitsu.com> writes:
> Tom Lane wrote:
>> After looking at the test case a bit, I have an alternate approach:
>> constraints on the column will have DEPENDENCY_AUTO type, while
>> constraints using the column will have DEPENDENCY_NORMAL type.
> O.K. It seems nicer than my solution.
> Please find the patch attached. I fixed the patch to decide the order
> of the list by deptype of the pg_depend record.
Patch applied with some minor comment improvements. Thanks!
regards, tom lane