285748 - Cross-DB bz_alter_column

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[Max Kanat-Alexander]

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.

Comment 1

[Max Kanat-Alexander]

Attachment: bz_alter_column

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

Comment 2

[Tomas Kopal]

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,

Comment 3

[Max Kanat-Alexander]

(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. :-(

Comment 4

[Max Kanat-Alexander]

Attachment: v2

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.)

Comment 5

[Max Kanat-Alexander]

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.
:-) )

Comment 6

[Max Kanat-Alexander]

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