Cross-DB bz_alter_column

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Bugzilla-General
P1
enhancement
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

2.19.2
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
9.50 KB, patch
Tomas Kopal
: review+
Details | Diff | Splinter Review
(Assignee)

Description

14 years ago
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.
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 1

14 years ago
Created attachment 177177 [details] [diff] [review]
bz_alter_column

OK, I only got through bz_alter_column. This, once again, also contains some
fixes to other patches.
Attachment #177177 - Flags: review?(Tomas.Kopal)
(Assignee)

Updated

14 years ago
Summary: Cross-DB bz_alter_column and bz_rename_column → Cross-DB bz_alter_column

Comment 2

14 years ago
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,
Attachment #177177 - Flags: review?(Tomas.Kopal) → review-
(Assignee)

Comment 3

14 years ago
(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. :-(
(Assignee)

Updated

14 years ago
Depends on: 285111, 285713
(Assignee)

Updated

14 years ago
Depends on: 285113
(Assignee)

Updated

14 years ago
Blocks: 285111
No longer depends on: 285111
(Assignee)

Comment 4

14 years ago
Created attachment 177721 [details] [diff] [review]
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.)
Attachment #177177 - Attachment is obsolete: true
Attachment #177721 - Flags: review?(Tomas.Kopal)

Updated

14 years ago
Attachment #177721 - Flags: review?(Tomas.Kopal) → review+
(Assignee)

Comment 5

14 years ago
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.
:-) )
(Assignee)

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 6

14 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

13 years ago
Blocks: 286695
You need to log in before you can comment on or make changes to this bug.