OK, now THIS one is going to be fun. PostgreSQL can't actually change the datatype of a column, so I'm going to have to do a whole fun deal with adding a temporary column, then doing an update with a CAST, etc.

OK, I only got through bz_alter_column. This, once again, also contains some fixes to other patches.

Comment on attachment 177177 [details] [diff] [review] bz_alter_column >--- mkanat/Bugzilla/DB/Schema/Mysql.pm 2005-03-11 01:08:53.000000000 -0800 >+++ mkanat4/Bugzilla/DB/Schema/Mysql.pm 2005-03-11 10:01:51.000000000 -0800 >@@ -96,5 +96,16 @@ >+ >+ return (("ALTER TABLE $table CHANGE COLUMN $column $column $new_ddl")); Are you sure you want the $column here twice? >--- mkanat/Bugzilla/DB/Schema.pm 2005-03-11 07:47:54.000000000 -0800 >+++ mkanat4/Bugzilla/DB/Schema.pm 2005-03-11 13:06:50.000000000 -0800 >@@ -1374,10 +1375,97 @@ >+=item C<get_alter_ddl($table, $column, \%definition)> >+ >+ \%definition - The new definition for the column, >+ >+ my ($self, $table, $column, $new_def) = @_; Why is the description using \%definition and the function $new_def? >+ # If the types have changed, we have to deal with that. >+ if (uc(trim($old_def->{TYPE})) ne uc(trim($new_def->{TYPE}))) { >+ $typechange = 1; >+ my $type = $new_def->{TYPE}; >+ $type = $specific->{$type} if exists $specific->{$type}; >+ # Make sure we can CAST from the old type to the new without an error. >+ push(@statements, "SELECT CAST($column AS $type) FROM $table LIMIT 1"); >+ # Add a new temporary column of the new type >+ push(@statements, "ALTER TABLE $table ADD COLUMN ${column}_ALTERTEMP" >+ . " $type"); >+ # UPDATE the temp column to have the same values as the old column >+ push(@statements, "UPDATE $table SET ${column}_ALTERTEMP = " >+ . " CAST($column AS $type)"); >+ # DROP the old column >+ push(@statements, "ALTER TABLE $table DROP COLUMN $column"); >+ # And rename the temp column to be the new one. >+ push(@statements, "ALTER TABLE $table RENAME COLUMN " >+ . " ${column}_ALTERTEMP TO $column"); >+ } This makes me wonder if we should not bite the bullet and require Pg 8.0, which supports column type changes (and it seems to be quite powerful). Also, what about indexes? Is it ok to keep existing index after changing the column type or do we have to drop it and create again? (My guess is we'll have to drop and recreate it.) And we are not even using foreign keys and other advanced stuff like that yet... >+ # If we went from NULL TO NOT NULL Nit: from NULL to NOT NULL >+ # If we went from being a PRIMARY KEY to not being a PRIMARY KEY, >+ # or if we changed types and we are a PK. from not being a PRIMARY KEY to being a PRIMARY KEY,

(In reply to comment #2) > >+ > >+ return (("ALTER TABLE $table CHANGE COLUMN $column $column $new_ddl")); > > Are you sure you want the $column here twice? Yep. Isn't MySQL weird? :-) > >+=item C<get_alter_ddl($table, $column, \%definition)> > >+ > >+ \%definition - The new definition for the column, > >+ > >+ my ($self, $table, $column, $new_def) = @_; > > Why is the description using \%definition and the function $new_def? Because I thought that definition was clearer to the caller, but new_def was easier to read in the code. > This makes me wonder if we should not bite the bullet and require Pg 8.0, > which supports column type changes (and it seems to be quite powerful). Well, I think we'd lose a lot of support. There are no distros yet shipping Pg 8, as far as I know. Also, we have no testing base for Pg 8, right now. I really think that the highest we ought to go is 7.4, if we have to go that high. 7.3 does everything we need, and it's what's on landfill. We have our Schema object, so we can deal with all the problems here. I've already dealt with most of them. > Also, what about indexes? Is it ok to keep existing index after changing the > column type or do we have to drop it and create again? (My guess is we'll have > to drop and recreate it.) And we are not even using foreign keys and other > advanced stuff like that yet... Ohhhh... you're right. Arrrgh. :-) You don't have to worry about foreign keys just yet, I think -- I could do an ALTER TABLE ONLY and get around that, maybe... The patch as posted also has a few other problems, but they're fixed in my local copy. Looks like the index stuff will block this stuff, though. :-(

OK, I've addressed the comments, but I haven't fixed the index stuff, yet. I did confirm that the index stuff is needed. However, pretty much none of bug 285111 will go in before any other part goes in, or at least none of it will be used until it's all checked in and I'm certain. So we can hold off on this part until I have bz_add_index, which should come up soon. (I'll add it as a part of that patch.)

Oh yeah, you're right. I'll deal with it in my next patch, though. :-) (Sorry, this is just an endless stream of patches to implement one thing, at this point. :-) )

Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.39; previous revision: 1.38 done Checking in Bugzilla/DB/Schema.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v <-- Schema.pm new revision: 1.13; previous revision: 1.12 done Checking in Bugzilla/DB/Schema/Mysql.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Mysql.pm,v <-- Mysql.pm new revision: 1.3; previous revision: 1.2 done Checking in Bugzilla/DB/Schema/Pg.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Pg.pm,v <-- Pg.pm new revision: 1.6; previous revision: 1.5 done