Bug 329537: [PostgreSQL] Bugzilla::DB::Pg can't alter a column to be SERIAL - Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit a=justdave

3 files changed, 43 insertions, 99 deletions

diff --git a/Bugzilla/DB/Pg.pm b/Bugzilla/DB/Pg.pm
index a8f54f903..5162466c5 100644
--- a/Bugzilla/DB/Pg.pm
+++ b/Bugzilla/DB/Pg.pm
@@ -50,10 +50,10 @@ use DBD::Pg;
 use base qw(Bugzilla::DB);
 
 use constant BLOB_TYPE => { pg_type => DBD::Pg::PG_BYTEA };
-use constant REQUIRED_VERSION => '7.03.0000';
+use constant REQUIRED_VERSION => '8.00.0000';
 use constant PROGRAM_NAME => 'PostgreSQL';
 use constant MODULE_NAME  => 'Pg';
-use constant DBD_VERSION  => '1.31';
+use constant DBD_VERSION  => '1.45';
 
 sub new {
     my ($class, $user, $pass, $host, $dbname, $port) = @_;
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index 44f17636c..32f09a099 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -1485,43 +1485,10 @@ sub get_alter_column_ddl {
     my $old_def = $self->get_column_abstract($table, $column);
     my $specific = $self->{db_specific};
 
-    my $typechange = 0;
     # 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)");
-
-        # Some databases drop a whole index when a column is dropped,
-        # some only remove that column from an index. For consistency,
-        # we manually drop all indexes on the column before dropping the
-        # column.
-        my %col_idx = $self->get_indexes_on_column_abstract($table, $column);
-        foreach my $idx_name (keys %col_idx) {
-            push(@statements, $self->get_drop_index_ddl($table, $idx_name));
-        }
-
-        # 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");
-
-        # And now, we have to regenerate any indexes that got
-        # dropped, except for the PK index which will be handled
-        # below.
-        foreach my $idx_name (keys %col_idx) {
-            push(@statements,
-                 $self->get_add_index_ddl($table, $idx_name, $col_idx{$idx_name}));
-        }
+        push(@statements, $self->_get_alter_type_sql($table, $column, 
+                                                     $new_def, $old_def));
     }
 
     my $default = $new_def->{DEFAULT};
@@ -1535,21 +1502,17 @@ sub get_alter_column_ddl {
         push(@statements, "ALTER TABLE $table ALTER COLUMN $column"
                         . " DROP DEFAULT");
     }
-    # If we went from no default to a default, or we changed the default,
-    # or we have a default and we changed the data type of the field
+    # If we went from no default to a default, or we changed the default.
     elsif ( (defined $default && !defined $default_old) || 
-         ($default ne $default_old) ||
-         ($typechange && defined $new_def->{DEFAULT}) ) {
+            ($default ne $default_old) ) 
+    {
         $default = $specific->{$default} if exists $specific->{$default};
         push(@statements, "ALTER TABLE $table ALTER COLUMN $column "
                          . " SET DEFAULT $default");
     }
 
-    # If we went from NULL to NOT NULL
-    # OR if we changed the type and we are NOT NULL
-    if ( (!$old_def->{NOTNULL} && $new_def->{NOTNULL}) ||
-         ($typechange && $new_def->{NOTNULL}) ) 
-    {
+    # If we went from NULL to NOT NULL.
+    if (!$old_def->{NOTNULL} && $new_def->{NOTNULL}) {
         my $setdefault;
         # Handle any fields that were NULL before, if we have a default,
         $setdefault = $new_def->{DEFAULT} if exists $new_def->{DEFAULT};
@@ -1570,10 +1533,8 @@ sub get_alter_column_ddl {
                         . " DROP NOT NULL");
     }
 
-    # If we went from not being a PRIMARY KEY to being a PRIMARY KEY,
-    # or if we changed types and we are a PK.
-    if ( (!$old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) ||
-         ($typechange && $new_def->{PRIMARYKEY}) ) {
+    # If we went from not being a PRIMARY KEY to being a PRIMARY KEY.
+    if (!$old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) {
         push(@statements, "ALTER TABLE $table ADD PRIMARY KEY ($column)");
     }
     # If we went from being a PK to not being a PK
diff --git a/Bugzilla/DB/Schema/Pg.pm b/Bugzilla/DB/Schema/Pg.pm
index 3361dc607..09e3bfbd4 100644
--- a/Bugzilla/DB/Schema/Pg.pm
+++ b/Bugzilla/DB/Schema/Pg.pm
@@ -90,70 +90,53 @@ sub _initialize {
 } #eosub--_initialize
 #--------------------------------------------------------------------
 
-# Overridden because Pg has such weird ALTER TABLE problems.
-sub get_add_column_ddl {
-    my ($self, $table, $column, $definition, $init_value) = @_;
+sub get_rename_column_ddl {
+    my ($self, $table, $old_name, $new_name) = @_;
 
-    # So that we don't change the $definition for the caller.
-    my $def = dclone($definition);
+    return ("ALTER TABLE $table RENAME COLUMN $old_name TO $new_name");
+}
 
+sub _get_alter_type_sql {
+    my ($self, $table, $column, $new_def, $old_def) = @_;
     my @statements;
-    my $specific = $self->{db_specific};
 
-    my $type = $def->{TYPE};
-    $type = $specific->{$type} if exists $specific->{$type};
+    my $type = $new_def->{TYPE};
+    $type = $self->{db_specific}->{$type} 
+        if exists $self->{db_specific}->{$type};
 
-    # SERIAL Types need special handlings
-    # XXX This will create a column that doesn't look like a
-    #     "SERIAL" in a pg_dump, but functions identically.
-    if ($type =~ /serial/i) {
+    if ($type =~ /serial/i && $old_def->{TYPE} !~ /serial/i) {
+        die("You cannot specify a DEFAULT on a SERIAL-type column.") 
+            if $new_def->{DEFAULT};
         $type =~ s/serial/integer/i;
-        $def->{DEFAULT} = "nextval('${table}_${column}_seq')";
-        push(@statements, "CREATE SEQUENCE ${table}_${column}_seq");
-        # On Pg, you don't need UNIQUE if you're a PK--it creates
-        # two identical indexes otherwise.
-        $type =~ s/unique//i if $def->{PRIMARYKEY};
     }
 
-    push(@statements, "ALTER TABLE $table ADD COLUMN $column $type");
-
-    my $default = $def->{DEFAULT};
-    if (defined $default) {
-        # Replace any abstract default value (such as 'TRUE' or 'FALSE')
-        # with its database-specific implementation.
-        $default = $specific->{$default} if exists $specific->{$default};
-        push(@statements, "ALTER TABLE $table ALTER COLUMN $column "
-                         . " SET DEFAULT $default");
-    }
+    # On Pg, you don't need UNIQUE if you're a PK--it creates
+    # two identical indexes otherwise.
+    $type =~ s/unique//i if $new_def->{PRIMARYKEY};
 
-    if (defined $init_value) {
-        push(@statements, "UPDATE $table SET $column = $init_value");
-    }
+    push(@statements, "ALTER TABLE $table ALTER COLUMN $column
+                              TYPE $type");
 
-    if ($def->{NOTNULL}) {
-        # Handle rows that were NULL when we added the column.
-        # We *must* have a DEFAULT. This check is usually handled
-        # at a higher level than this code, but I figure it can't
-        # hurt to have it here.
-        die "NOT NULL columns must have a DEFAULT or an init_value."
-            unless (exists $def->{DEFAULT} || defined $init_value);
-        push(@statements, "UPDATE $table SET $column = $default");
-        push(@statements, "ALTER TABLE $table ALTER COLUMN $column "
-                         . " SET NOT NULL");
+    if ($new_def->{TYPE} =~ /serial/i && $old_def->{TYPE} !~ /serial/i) {
+        push(@statements, "CREATE SEQUENCE ${table}_${column}_seq");
+        push(@statements, "SELECT setval('${table}_${column}_seq',
+                                         MAX($table.$column))");
+        push(@statements, "ALTER TABLE $table ALTER COLUMN $column 
+                           SET DEFAULT nextval('${table}_${column}_seq')");
     }
 
-    if ($def->{PRIMARYKEY}) {
-         push(@statements, "ALTER TABLE $table "
-                         . " ADD PRIMARY KEY ($column)");
+    # If this column is no longer SERIAL, we need to drop the sequence
+    # that went along with it.
+    if ($old_def->{TYPE} =~ /serial/i && $new_def->{TYPE} !~ /serial/i) {
+        push(@statements, "ALTER TABLE $table ALTER COLUMN $column 
+                           DROP DEFAULT");
+        # XXX Pg actually won't let us drop the sequence, even though it's
+        #     no longer in use. So we harmlessly leave behind a sequence
+        #     that does nothing.
+        #push(@statements, "DROP SEQUENCE ${table}_${column}_seq");
     }
 
     return @statements;
 }
 
-sub get_rename_column_ddl {
-    my ($self, $table, $old_name, $new_name) = @_;
-
-    return ("ALTER TABLE $table RENAME COLUMN $old_name TO $new_name");
-}
-
 1;