285722 - Updates From 2.18- to 2.20+ will not work

Status: RESOLVED FIXED

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

Description

[Max Kanat-Alexander]

2.20+ will use the new method of the serialized Schema for updates. However,
previous versions don't.

So the wrong serialized schema will be written to the database upon an initial
update.

So, for MySQL *only*, if the serialized schema doesn't exist, we need to read
the database from the disk and create a serialized schema.

Thankfully, we have all the functions necessary to do this already. We just need
a reverse mapping in Schema::Mysql and a _build_initial_schema function in that
module.

This is thankfully possible for MySQL, because the mappings in Schema are pretty
much one-to-one. To differentiate between INT1 and tinyint, we can have a custom
map.

Comment 1

[Max Kanat-Alexander]

I mean, between INT1 and BOOLEAN. :-)

We'll also have to have some special handling for TIMESTAMP fields.

Comment 2

[Max Kanat-Alexander]

Oh, arrgh. Actually, it means that I *must* re-write checksetup to use the new
code. Ahh, well. :-) It can be done. Thankfully I have that
checksetup-regression script. :-)

Comment 3

[Max Kanat-Alexander]

Attachment: v0.9 (use -p1) (requires Bug 287986)

OK, here's what I have so far, so that you can get an idea.

Basically, this converts the whole of checksetup to use the new Schema methods
of changing the database.

In a few places, you'll notice that I've made very minor additions -- the new
Schema methods are much more strict than their old MySQL-specific versions, and
they demand that you play nice when adding new columns or altering old ones. In
particular, they complain if you try to make NOT NULL things that shouldn't be
NOT NULL. (Things that have no default or initial value, or columns being
altered that currently contain NULLs.)

I've also cleaned up some things in Bugzilla::DB, as this will be pretty much
the final patch in this area, and a few things proved themselves to not be
working when actually tested with this code.

I still have to replace the three manual ALTER TABLE statements that are left
in checksetup.

I haven't *fully* tested this patch yet, but I know that it can upgrade from
every major release since 2.8. Various things like schema consistency I'm going
to be testing soon, and I'll also be running a full test-checksetup regression
test.

I'm mostly posting it because it's MASSIVE, and if you (anybody) want to start
looking over it now for obvious "oops" things, or for any architectural issues,
please do. :-)

I have also removed all the old Schema-checking/updating methods.

Comment 4

[Max Kanat-Alexander]

Oh, and I forgot to mention -- I left my debugging code in, so it prints a bit
of extra information when run.

Comment 5

[Tomas Kopal]

(In reply to comment #3)
> Created an attachment (id=180033) [edit]
> v0.9 (use -p1) (requires Bug 287986)
> 
> OK, here's what I have so far, so that you can get an idea.
> 

I have just very quickly went over this and it looks ok. But as you said, it's
massive. What about breaking it up into multiple pieces? ;-)
Big patch just renaming function or swapping arguments is ok, but if the actual
functionality is changed, I would prefer smaller bits to be able to concentrate
on them. (And it's ok if all pieces would have to be checked in together).

Comment 6

[Max Kanat-Alexander]

Attachment: v1 (requires Bug 287986)

OK. This patch changes all of checksetup to use the new Schema methods instead
of the old ones.

I haven't yet run a full regression test on it, but I've tried to upgrade from
all recent major versions and it works. I'm running the full regression test
now, but it takes a while, so I figured I'd post the patch and reviews could
happen at the same time as the test is running.

There are a few changes that aren't the standard checksetup changes:

+ I removed all the deprecated schema functions from Bugzilla::DB
+ There were a few bugfixes to be done in Bugzilla::DB for functions that had
  never been fully tested before this patch.
+ The index on flags.type_id was originally added in checksetup without a name,

  so we have to drop the old misnamed one.
+ The location of where we call "$self->SUPER::bz_setup_database();" in 
  Bugzilla::DB::Mysql::bz_setup_database was changed. This is so that we can
  use the new Schema methods instead of the old ones for the timestamp changes.

+ Fixed a typo in index renaming that I somehow never noticed before. (This can

  go in another bug if you *really* want...)
+ The fix for the possible PRIMARY index on target_milestones is no longer 
  necessary, as all such problems will be fixed by bug 290277.

Comment 7

[Tomas Kopal]

Comment on attachment 181230 [details] [diff] [review]
v1 (requires Bug 287986)

>+$dbh->bz_add_column('components', 'initialqacontact', 
>+                    {TYPE => 'TINYTEXT'});

TINYTEXT => INT3

>-    $dbh->bz_lock_tables('bugs write', 'longdescs write', 'profiles write');
>+    $dbh->bz_lock_tables('bugs write', 'longdescs write', 'profiles write',
>+                         'bz_schema WRITE');

This should not be neccessary, as (hopefully) noone else than checksetup will
touch bz_schema table, and noone will run checksetup twice. But it's a nice
sanity check.

>     $dbh->bz_lock_tables('bugs_activity WRITE', 'fielddefs WRITE');
>@@ -2121,7 +2127,7 @@
>     }
>     $dbh->bz_unlock_tables();
> 
>-    $dbh->bz_drop_field('bugs_activity', 'field');
>+    $dbh->bz_drop_column('bugs_activity', 'field');
> }

But you need to be consistent then and lock it always... (it's on couple of
other places where you are dropping columns and not locking, I will not
enumerate them here).

>-if ( $dbh->bz_get_index_count('cc') != 3 ) {
>+if (!$dbh->bz_index_info('cc', 'cc_bug_id_idx')->{TYPE}) {
> 
>     # XXX should eliminate duplicate entries before altering
>     #
>-    print "Recreating indexes on cc table.\n";
>-    $dbh->bz_drop_table_indexes('cc');
>-    $dbh->do("CREATE UNIQUE INDEX cc_bug_id_idx ON cc(bug_id,who)");
>-    $dbh->do("CREATE INDEX cc_who_idx ON cc(who)");
>+    $dbh->bz_drop_index('cc', 'cc_bug_id_idx');
>+    $dbh->bz_add_index('cc', 'cc_bug_id_idx', 
>+                      {TYPE => 'UNIQUE', FIELDS => [qw(bug_id who)]});
> }    

This looks to me like functional change and I don't fully understand
consequences (e.g. is the only index to drop the cc_bug_id_idx? The old code is
dropping all of them). Please explain.

>-if ( $dbh->bz_get_index_count('keywords') != 3 ) {
>+if (!$dbh->bz_index_info('keywords', 'keywords_bug_id_idx')->{TYPE}) {
> 
>     # XXX should eliminate duplicate entries before altering
>     #
>-    print "Recreating indexes on keywords table.\n";
>-    $dbh->bz_drop_table_indexes('keywords');
>-    $dbh->do("CREATE UNIQUE INDEX keywords_bug_id_idx"
>-             . " ON keywords(bug_id,keywordid)");
>-    $dbh->do("CREATE INDEX keywords_keywordid_idx ON keywords(keywordid)");
>-
>+    $dbh->bz_drop_index('keywords', 'keywords_bug_id_idx');
>+    $dbh->bz_add_index('keywords', 'keywords_bug_id_idx', 
>+                      {TYPE => 'UNIQUE', FIELDS => [qw(bug_id keywordid)]});
> }    

Dtto.

>-# 2000-11-27 For Bugzilla 2.5 and later. Change table 'comments' to 
>+# 2000-11-27 For Bugzilla 2.5 and later. Copy data from 'comments' to
> # 'longdescs' - the new name of the comments table.
>-if ($dbh->bz_table_exists('comments')) {
>-    $dbh->bz_rename_field('comments', 'when', 'bug_when');
>-    $dbh->bz_change_field_type('comments', 'bug_id', 'mediumint not null');
>-    $dbh->bz_change_field_type('comments', 'who', 'mediumint not null');
>-    $dbh->bz_change_field_type('comments', 'bug_when', 'datetime not null');
>-    $dbh->bz_rename_field('comments','comment','thetext');
>-    # Here we rename comments to longdescs
>-    $dbh->do("DROP TABLE longdescs");
>-    $dbh->do("ALTER TABLE comments RENAME longdescs");
>+if ($dbh->bz_table_info('comments')) {
>+    my $quoted_when = $dbh->quote_identifier('when');
>+    # This is MySQL-specific syntax, but that's OK because it will only
>+    # ever run on MySQL.
>+    $dbh->do("INSERT INTO longdescs (bug_when, bug_id, who, thetext)
>+              SELECT $quoted_when, bug_id, who, comment
>+                FROM comments");
>+    $dbh->bz_drop_table("comments");
> }

Again functional change. Will the longdesc table have always the correct
columns? Old code was converting the table, yours just rely on the table to
exist and be correct. What if, for example, we change the schema of the table
in the Schema.pm in the future? Will we need special conversion code before and
after this update to make sure the table is as we expect it to be now?

>-$dbh->bz_add_field('attachments', 'isobsolete', 'tinyint not null default 0');
>+$dbh->bz_add_column('attachments', 'isobsolete', 
>+                    {TYPE => 'INT1', NOTNULL => 1, DEFAULT => 'FALSE'});

INT1 => BOOLEAN

>-$dbh->bz_change_field_type("profiles", "disabledtext", "mediumtext not null");
>+$dbh->do("UPDATE profiles SET disabledtext = '' WHERE disabledtext IS NULL");
>+$dbh->bz_alter_column("profiles", "disabledtext", 
>+                      {TYPE => 'MEDIUMTEXT', NOTNULL => 1});

Can't we just specify initial value to bz_alter_column and skip the UPDATE? Is
it worth adding a new optional param to bz_alter_column? (There is a lot of
other places where you are calling bz_alter_column and changing the type to NOT
NULL, although they do not have any update there at the moment. If you decide
to add the parameter, please review the other calls as well. I would personaly
prefer it, as it makes the conversion more robust).

>-if (!$dbh->bz_get_field_def("bugs", "alias")) {
>-    $dbh->bz_add_field("bugs", "alias", "VARCHAR(20)");
>-    $dbh->do("CREATE UNIQUE INDEX bugs_alias_idx ON bugs(alias)");
>+if (!$dbh->bz_column_info("bugs", "alias")) {
>+    $dbh->bz_add_column("bugs", "alias", {TYPE => "varchar(20)"});
>+    $dbh->bz_add_index('bugs', 'bugs_alias_idx', [qw(alias)]);
> }

Shouldn't the index be UNIQUE?

>-if (!$dbh->bz_get_field_def('longdescs', 'already_wrapped')) {
>-    $dbh->bz_add_field('longdescs', 'already_wrapped', 'tinyint not null default 0');
>+if (!$dbh->bz_column_info('longdescs', 'already_wrapped')) {
>     # Old, pre-wrapped comments should not be auto-wrapped
>-    $dbh->do('UPDATE longdescs SET already_wrapped = 1');
>+    $dbh->bz_add_column('longdescs', 'already_wrapped',
>+        {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'}, 1);

Shouldn't the initial value be 'TRUE' instead of 1 here? Hmmm, maybe not...

> # 2005-03-27 initialqacontact should be NULL instead of 0, bug 287483
>-if (!$dbh->bz_get_field_def('components',
>-                            'initialqacontact')->[2]) { # if it's NOT NULL
>-    $dbh->bz_change_field_type('components', 'initialqacontact', 'mediumint');
>-    $dbh->do("UPDATE components SET initialqacontact = NULL " .
>+if ($dbh->bz_column_info('components', 'initialqacontact')->{NOTNULL}) {
>+    $dbh->bz_alter_column('components', 'initialqacontact', {TYPE => 'INT3'});      $dbh->do("UPDATE components SET initialqacontact = NULL " .
>              "WHERE initialqacontact = 0");

Newline here, please.

>-if ($dbh->bz_get_field_def("profiles", "emailflags")) {    
>-    print "Migrating email preferences to new table ...\n" unless $silent;
>+if ($dbh->bz_column_info("profiles", "emailflags")) {
>+print "Migrating email preferences to new table ...\n" unless $silent;

And indent here.

>@@ -3848,23 +3897,32 @@
> } 
> 
> # 2005-03-27: Standardize all boolean fields to plain "tinyint"
>-if ($dbh->bz_get_field_def('quips', 'approved')->[1] eq 'tinyint(1)') {
>-    $dbh->bz_change_field_type('quips', 'approved', 
>-                               'tinyint not null default 1');
>-    $dbh->bz_change_field_type('series', 'public', 
>-                               'tinyint not null default 0');
>-    $dbh->bz_change_field_type('bug_status', 'isactive', 
>-                               'tinyint not null default 1');
>-    $dbh->bz_change_field_type('rep_platform', 'isactive', 
>-                               'tinyint not null default 1');
>-    $dbh->bz_change_field_type('resolution', 'isactive', 
>-                               'tinyint not null default 1');
>-    $dbh->bz_change_field_type('op_sys', 'isactive', 
>-                               'tinyint not null default 1');
>-    $dbh->bz_change_field_type('bug_severity', 'isactive', 
>-                               'tinyint not null default 1');
>-    $dbh->bz_change_field_type('priority', 'isactive', 
>-                               'tinyint not null default 1');
>+if ( $dbh->isa('Bugzilla::DB::Mysql') ) {
>+  # This is a change to make things consistent with Schema, so we use
>+  # direct-database access methods.
>+  my $quip_info_sth = $dbh->column_info(undef, undef, 'quips', '%');
>+  my $quips_cols    = $quip_info_sth->fetchall_hashref("COLUMN_NAME");
>+  my $approved_col  = $quips_cols->{'approved'};
>+  if ( $approved_col->{TYPE_NAME} eq 'TINYINT'
>+       and $approved_col->{COLUMN_SIZE} == 1 )

Please indent more here to keep it consistent with the rest of the code.

>+$dbh->bz_add_index('flags', 'flags_type_id_idx', ['type_id']);

You are using qw for the indexed column everywhere else, please change it here
to make it consistent.


Ahhhh, it's huge. Don't want to do it again ;-). But it looks quite good, I
like it.

Comment 8

[Tomas Kopal]

(In reply to comment #7)
> (From update of attachment 181230 [details] [diff] [review] [edit])
> >+$dbh->bz_add_column('components', 'initialqacontact', 
> >+                    {TYPE => 'TINYTEXT'});
> 
> TINYTEXT => INT3

Actually, ignore this one, it's ok :-).

Comment 9

[Max Kanat-Alexander]

> >-    $dbh->bz_lock_tables('bugs write', 'longdescs write', 'profiles write');
> >+    $dbh->bz_lock_tables('bugs write', 'longdescs write', 'profiles write',
> >+                         'bz_schema WRITE');
> 
> This should not be neccessary, [snip]

  Actually, otherwise MySQL complains that you tried to access a table that you
didn't lock. :-)

  Eventually I want the DB-upgrade porition of checksetup in a lock on *all* the
tables, but that's for another bug. (I'll create a bz_lock_database function.)

> But you need to be consistent then and lock it always... (it's on couple of
> other places where you are dropping columns and not locking, I will not
> enumerate them here).

  Actually, I found one other place where there's already a bz_lock_tables call,
but there's no schema mods inside it, so it doesn't need to be modified.

> >+    $dbh->bz_drop_index('cc', 'cc_bug_id_idx');
> >+    $dbh->bz_add_index('cc', 'cc_bug_id_idx', 
> >+                      {TYPE => 'UNIQUE', FIELDS => [qw(bug_id who)]});
> > }    
> 
> This looks to me like functional change and I don't fully understand
> consequences (e.g. is the only index to drop the cc_bug_id_idx? The old code 
> is dropping all of them). Please explain.

  Yeah, the only index to drop at that point in time is the cc_bug_id_idx. I
went back and researched the old Schema. The old code was just being lazy. I
didn't want to also write a bz_drop_all_indexes function if I could avoid it,
because that really wasn't a good way to go about things. And I did manage to
avoid it. :-)

> >+    $dbh->bz_drop_index('keywords', 'keywords_bug_id_idx');
> >+    $dbh->bz_add_index('keywords', 'keywords_bug_id_idx', 
> >+                      {TYPE => 'UNIQUE', FIELDS => [qw(bug_id keywordid)]});
> > }    
> 
> Dtto.

  Yeah, and the same for this bit. I researched all the indexes that could
possibly exist on that table at that time, and the ones I dropped are the ones
that need to be dropped. I even created a whole new database on landfill and
added it to test-checksetup to make sure that this code section actually runs.
(The "keywords" bit will only run if you were using a 2.9 CVS version.)

> >-# 2000-11-27 For Bugzilla 2.5 and later. Change table 'comments' to 
> >+# 2000-11-27 For Bugzilla 2.5 and later. Copy data from 'comments' to
> > [snip]
> Again functional change. 

  Yeah. I didn't want to implement a bz_rename_table function that we'd never
need outside of this code, which is designed to only run on Bugzilla 2.5.

> Will the longdesc table have always the correct
> columns? Old code was converting the table, yours just rely on the table to
> exist and be correct. [snip]

  That's true. So it's technically possible that we could re-work the longdescs
table in the future and have to re-work this little code section.

  Of course, this is only for (totally untestable) upgrades from 2.5, so I'm
really not that worried about it. When the, like, *one* person in the world who
is somewhere still using 2.5 complains that it doesn't work some day, we can fix
it. :-) And then, we can tell them to upgrade to 2.18 first and then to 2.20+,
if we really want.

> INT1 => BOOLEAN

  Oh, good catch! Thanks!

> Can't we just specify initial value to bz_alter_column and skip the UPDATE? Is
> it worth adding a new optional param to bz_alter_column? (There is a lot of
> other places where you are calling bz_alter_column and changing the type to 
> NOT NULL, although they do not have any update there at the moment. If you 
> decide to add the parameter, please review the other calls as well. I would 
> personaly prefer it, as it makes the conversion more robust).

  Yeah, I suppose you're right, we should add that parameter -- it would make
the calls more robust, now that they enforce the NOT NULL restriction where they
didn't before.
 
> Shouldn't the index be UNIQUE?

  Yeah, my regression tests caught that, too. :-)

> Shouldn't the initial value be 'TRUE' instead of 1 here? Hmmm, maybe not...

  It turns out to be much more complex code if I have to have $init_value be
translated through the filter, so I just let it be "1." I'm not really a big fan
of the whole "TRUE"/"FALSE" thing in Schema anyway, since we can't use real
Boolean types in any DB right now without major other modifications of Bugzilla.

> Newline here, please.

  Oh, thanks! That was a funny pasting error.

> And indent here.

  Also a good catch. :-)
 
> Please indent more here to keep it consistent with the rest of the code.

  And again. :-)

> You are using qw for the indexed column everywhere else, please change it here
> to make it consistent.

  Oh, thanks. :-) Yeah, it was my intention to use qw everywhere, I believe.

> Ahhhh, it's huge. Don't want to do it again ;-). 

  Hahahahaha, I understand. This was pretty much the smallest it could get,
since it's totally untestable as little parts.

> But it looks quite good, I like it.

  Thanks. :-) The regression tests showed up a few more errors, too -- one that
I'm still trying to figure out, where on upgrades from 2.8 the
milestones_product_id_idx loses the product_id column...

Comment 10

[Max Kanat-Alexander]

Oh, I also discovered that if you tried to rename a PRIMARY KEY in MySQL, the
code would die because it thought you were trying to create a duplicate PK. So
the new patch will also include a fix for that.

Comment 11

[Max Kanat-Alexander]

Attachment: v2 (requires Bug 287986)

OK, I've addressed all your comments in this one. :-) I didn't do a "cvs
update" in between this patch and the last one, so the interdiff should be
accurate, I believe.

I added the extra parameter to bz_alter_column, and I also made use of it in
checksetup everywhere that I found us changing from a column that *could*
contain a NULL. (It wasn't actually that frequent of a situation.)

Comment 12

[Tomas Kopal]

Comment on attachment 181514 [details] [diff] [review]
v2 (requires Bug 287986)

(In reply to comment #11)
> Created an attachment (id=181514) [edit]
> v2 (requires Bug 287986)
> 
> OK, I've addressed all your comments in this one. :-) I didn't do a "cvs
> update" in between this patch and the last one, so the interdiff should be
> accurate, I believe.

Thanks. it is accurate, but still quite long ;-).

> 
> I added the extra parameter to bz_alter_column, and I also made use of it in
> checksetup everywhere that I found us changing from a column that *could*
> contain a NULL. (It wasn't actually that frequent of a situation.)

Well, I would probably add it to all calls to bz_alter_column, where we specify
NOT NULL and no default, just to play safe and for the sake of completenes, but
if you are sure you got all the important ones, it's ok. I works, after all,
doesn't it? :-)

>@@ -1422,6 +1273,11 @@
>               $name    = the name of the column you want to change
>               $new_def = An abstract column definition for the new 
>                          data type of the columm
>+              $set_nulls_to = (Optional) If you are changing the column
>+                              to be NOT NULL, you probably also want to
>+                              set any existing NULL columns to a particular
>+                              value. Specify that value here. The value
>+                              does not need to already be SQL-quoted.

I would say that the value should not be sql-quoted, as if it is, you will end
up with multiple quotes, which, at least for strings, is probably not what you
want. So I would stress it bit more here.

>@@ -151,9 +151,14 @@
> 
> # MySQL has a simpler ALTER TABLE syntax than ANSI.
> sub get_alter_column_ddl {
>-    my ($self, $table, $column, $new_def) = @_;
>+    my ($self, $table, $column, $new_def, $set_nulls_to) = @_;
>     my $new_ddl = $self->get_type_ddl($new_def);
>-    return (("ALTER TABLE $table CHANGE COLUMN $column $column $new_ddl"));
>+    my @statements;
>+    push(@statements, "UPDATE $table SET $column = $set_nulls_to") 
>+        if defined $set_nulls_to;

I am pretty sure you want "WHERE $column IS NULL" here :-).

>@@ -281,6 +286,8 @@
> sub get_rename_column_ddl {
>     my ($self, $table, $old_name, $new_name) = @_;
>     my $def = $self->get_type_ddl($self->get_column($table, $old_name));
>+    # MySQL doesn't like having the PRIMARY KEY statement in a rename.
>+    $def =~ s/PRIMARY KEY//;

Just to play safe, make it case insensitive here, please.

Good, it's almost complete :-).

Comment 13

[Max Kanat-Alexander]

Attachment: v3

OK, I realized that you were right about the sql-quoting of the $init_value
params and the $set_nulls_to params, so I stopped passing them in as "''", and
just switched them to being ''.

I also addressed the case-sensitivity thing and the WHERE $column IS NULL.
(Good catch!) :-)

Comment 14

[Tomas Kopal]

Comment on attachment 181576 [details] [diff] [review]
v3

Good stuff.

Comment 15

[Max Kanat-Alexander]

I'd like to get this in and then pretty much release 2.19.3 a day or two after,
if we can. Before 2.19.3 goes out, it might be good to run the "new" checksetup
on a few real databases.

Comment 16

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.398; previous revision: 1.397
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.52; previous revision: 1.51
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.17; previous revision: 1.16
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.26; previous revision: 1.25
done
Checking in Bugzilla/DB/Schema/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Mysql.pm,v  <--  Mysql.pm
new revision: 1.8; previous revision: 1.7
done

Comment 17

[Marc Schumann [:Wurblzap]]

*** Bug 313896 has been marked as a duplicate of this bug. ***

Comment 18

[Max Kanat-Alexander]

*** Bug 313896 has been marked as a duplicate of this bug. ***