310231 - MySQL-specific get_alter_column_ddl will not drop primary key

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Olav Vitters]

In Bugzilla/DB/Schema/Mysql.pm the get_alter_column_ddl is overridden. This
function assumes MySQL will handle primary key changes (dropping it). MySQL
(tested with 4.1.12 and 4.1.14) doesn't do this, it only does the column type
changes.

This bug can be seen by applying the attachment.cgi 197620 from bug 119524. Note
that I also saw another MySQL bug (details in bug 119524 comment 17). For that
MySQL bug I'm going to file another Bugzilla bug to workaround it.

Fix for this bug should be as easy as adding the relevant primary key parts from
get_alter_column_ddl from Bugzilla/DB/Schema.pm.

Comment 1

[Max Kanat-Alexander]

Yeah, I knew about this one. :-) We should test the Pg code to see if it is also
broken, because we've never had to change a PK in the history of Bugzilla. :-)

Comment 2

[Olav Vitters]

Attachment: MySQL

Using ALTER TABLE and PRIMARY KEY in MySQL works as follows:
1. Specifying 'PRIMARY KEY' in ALTER TABLE $table CHANGE COLUMN will make MySQL
add a primary key. If that column already was the primary key, MySQL will
produce an error message! Note that leaving out 'PRIMARY KEY' is ok, even with
type changes the column will be a PRIMARY KEY.
2. To drop a 'PRIMARY KEY', you cannot just leave out the 'PRIMARY KEY' from
the 'ALTER TABLE $table CHANGE COLUMN'-statement. MySQL requires an 'ALTER
TABLE $table DROP PRIMARY KEY'.
3. Adding a primary key CAN be done by specifying 'PRIMARY KEY' as usual (in
the CHANGE COLUMN statement).

The patch fully allows the primary key columns to be changed under MySQL.

Comments on the code:
If the column stays a primary key it should not specify 'PRIMARY KEY' on the
ALTER TABLE. I did this by changing the hash given to get_type_ddl(). I had to
make a copy of the hash, because the hash is passed by reference. Other
function that run after get_alter_column_ddl would otherwise still see the hash
without the {PRIMARYKEY} set. 


=== Postgresql ===
From my investigations, the get_alter_column_ddl from Bugzilla/DB/Schema.pm
will NOT drop the primary key under Postgresql. Postgresql does not support the
used 'ALTER TABLE $table DROP PRIMARY KEY'. Postgresql uses the syntax:
  ALTER TABLE $table DROP CONSTRAINT some_name;

This requires Bugzilla to pass the constraint name. The default primary key
constraint name is (perl):
  $table . "_pkey"
Example:
  logincookies_pkey for the table logincookies
Postgresql actually produces warnings that these constraints are created when
making the database. Although these constraints can have other names, this will
not happen if the tables have been created by Bugzilla. I'm not sure though if
this is standard SQL (and thus belongs in the main get_alter_column_ddl
function), or if this should be moved to a Pg-specific function (+ how to
cleanly do this).

I've changed the patch in bug 119524 to keep the primary key (still requires
this patch to work with MySQL, see #1 at the top of this comment). This has an
additional benefit of working fine with Pg (as no primary key is dropped).

Comment 3

[Olav Vitters]

Comment on attachment 197714 [details] [diff] [review]
MySQL 

(read my request in bug 119524 first if you haven't)

This bug fixes the MySQL primary key changes. In my initial patch for 119524 I
needed MySQL to drop the primary key (which it did not). For the newest patch I
keep the primary key, however,	still needs fixes to make it work. See comments
for more info. This is all MySQL specific. Postgresql dropping of a primary key
I can do, but I'd like to do that in another bug as it is not required anymore
by the latest patch in bug 119524. See comments in this bug for more info.

I have tested the drop primary key using attachment 197620 [details] [diff] [review] (the old obsolete
one of bug 119524). The other stuff I've tested using the latest patch in
119524 (requested review on that from you).

Comment 4

[Max Kanat-Alexander]

Comment on attachment 197714 [details] [diff] [review]
MySQL 

> # MySQL has a simpler ALTER TABLE syntax than ANSI.
> sub get_alter_column_ddl {
>     my ($self, $table, $column, $new_def, $set_nulls_to) = @_;
>-    my $new_ddl = $self->get_type_ddl($new_def);
>+    my $old_def = $self->get_column_abstract($table, $column);

  You want get_column, not get_column_abstract.

>+    my %new_def_copy = %$new_def;
>+    if ($old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) {

  Make sure that doesn't throw any "undefined blah" errors.

>+    my $new_ddl = $self->get_type_ddl(\%new_def_copy);
>     my @statements;
>     push(@statements, "UPDATE $table SET $column = $set_nulls_to
>                         WHERE $column IS NULL") if defined $set_nulls_to;
>     push(@statements, "ALTER TABLE $table CHANGE COLUMN 
>                        $column $column $new_ddl");

>+    if ($old_def->{PRIMARYKEY} && !$new_def->{PRIMARYKEY}) {
>+        # Dropping a PRIMARY KEY needs an explicit DROP PRIMARY KEY
>+        push(@statements, "ALTER TABLE $table DROP PRIMARY KEY");
>+    }

  Looks good. And what about adding a PK?

Comment 5

[Max Kanat-Alexander]

Hey Olav, do you have an updated patch for this? I'd really like to see it in 2.22, if possible.

Comment 6

[Olav Vitters]

Thanks for reminding me. Was a bit busy.

(In reply to comment #4)
>   You want get_column, not get_column_abstract.

Did that.

> >+    my %new_def_copy = %$new_def;
> >+    if ($old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) {
> 
>   Make sure that doesn't throw any "undefined blah" errors.

Not exactly sure what I should test for. $old_def && $old_def->{PRIMARYKEY}? Or defined $old_def->{PRIMARYKEY} && $old_def->{PRIMARYKEY}? I thought the last one wouldn't give an undefined error (because of the '->' ?). 

> >+    my $new_ddl = $self->get_type_ddl(\%new_def_copy);
> >     my @statements;
> >     push(@statements, "UPDATE $table SET $column = $set_nulls_to
> >                         WHERE $column IS NULL") if defined $set_nulls_to;
> >     push(@statements, "ALTER TABLE $table CHANGE COLUMN 
> >                        $column $column $new_ddl");
> 
> >+    if ($old_def->{PRIMARYKEY} && !$new_def->{PRIMARYKEY}) {
> >+        # Dropping a PRIMARY KEY needs an explicit DROP PRIMARY KEY
> >+        push(@statements, "ALTER TABLE $table DROP PRIMARY KEY");
> >+    }
> 
>   Looks good. And what about adding a PK?

MySQL will always add a primary key when the ALTER TABLE specifies 'PRIMARY KEY'. When the column already is a primary key, this will cause a MySQL error.

What this patch do is *only* remove the 'PRIMARY KEY from the 'ALTER TABLE' statement if:
    old_column is a primary key
AND new_column is a primary key

When adding a primary key the old_column will not be a primary key. So the 'ALTER TABLE' statement will still have 'PRIMARY KEY' for the column and MySQL will make the column a primary key (I have tested this).

Comment 7

[Olav Vitters]

Attachment: Patch v2: CVS HEAD 2005-12-17

(I'm almost reposting my earlier comment so it is clearer in the flag notification)

(In reply to comment #4)
>   You want get_column, not get_column_abstract.

Did that.

> >+    my %new_def_copy = %$new_def;
> >+    if ($old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) {
> 
>   Make sure that doesn't throw any "undefined blah" errors.

Can't see what could throw undefined here. I asked you on IRC and you didn't know. :)

The if-check is like an if-check in Bugzilla/DB/Schema.pm, also get_alter_column_ddl:
> if ( (!$old_def->{PRIMARYKEY} && $new_def->{PRIMARYKEY}) ||
>      ($typechange && $new_def->{PRIMARYKEY}) ) {

So I'm sure it won't throw undefined errors.

> >+    my $new_ddl = $self->get_type_ddl(\%new_def_copy);
> >     my @statements;
> >     push(@statements, "UPDATE $table SET $column = $set_nulls_to
> >                         WHERE $column IS NULL") if defined $set_nulls_to;
> >     push(@statements, "ALTER TABLE $table CHANGE COLUMN 
> >                        $column $column $new_ddl");
> 
> >+    if ($old_def->{PRIMARYKEY} && !$new_def->{PRIMARYKEY}) {
> >+        # Dropping a PRIMARY KEY needs an explicit DROP PRIMARY KEY
> >+        push(@statements, "ALTER TABLE $table DROP PRIMARY KEY");
> >+    }
> 
>   Looks good. And what about adding a PK?

MySQL will always add a primary key when the ALTER TABLE specifies 'PRIMARY
KEY'. When the column already is a primary key, this will cause a MySQL error.

What this patch do is *only* remove the 'PRIMARY KEY from the 'ALTER TABLE'
statement if:
    old_column is a primary key
AND new_column is a primary key

When adding a primary key the old_column will not be a primary key. So the
'ALTER TABLE' statement will still have 'PRIMARY KEY' for the column and MySQL
will make the column a primary key (I have tested this).

Comment 8

[Max Kanat-Alexander]

Comment on attachment 206183 [details] [diff] [review]
Patch v2: CVS HEAD 2005-12-17

Yep, looks good and tests out on a few test upgrades.

Comment 9

[Max Kanat-Alexander]

Checking in Bugzilla/DB/Schema/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Mysql.pm,v  <--  Mysql.pm
new revision: 1.12; previous revision: 1.11
done

Comment 10

[Max Kanat-Alexander]

Since we have decided not to backport bug 119524, this doesn't need to be
backported either.