284850 - checksetup should rename indexes to conform to the new standard

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, enhancement, P1)

Description

[Max Kanat-Alexander]

We have changed all the index names in the bz-dbschema bug. To be specific,
we've actually *given* them names.

This makes life much easier. However, current Bugzillas have names for the
indexes equal to the first column that the index is on.

In order to get consistency (and so I can eliminate bz_get_index_by_field -- see
bug 284845), we need to rename all the old indexes to the new format.

This only needs to be done on MySQL, so we don't need a bz_rename_index function.

Hopefully, there's some fast way to do this, and we don't have to drop the
indexes and re-create them...?

Comment 1

[:glob ✱]

RENAME INDEX <old_index_name> [ON <table_name>] TO <new_index_name>

Comment 2

[Max Kanat-Alexander]

Attachment: Rename old indexes on MySQL

Welcome to the land of Bugzilla::DB. :-D

The patch pretty much explains itself in comments.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #1)
> RENAME INDEX <old_index_name> [ON <table_name>] TO <new_index_name>

  That only works for MaxDB, unfortunately.

Comment 4

[Max Kanat-Alexander]

OK, I forgot to mention that this patch needs the patch on bug 286689 in order
to function properly.

Comment 5

[Jouni Heikniemi]

Before I review in detail, don't you think we should have better
pre-warning/confirmation for a database operation that can take hours?
Particularly if the abortability/continuability of the operation is sub-perfect.
(I don't know, I only quickly browsed through the code so far)

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> Before I review in detail, don't you think we should have better
> pre-warning/confirmation for a database operation that can take hours?

  Oh, definitely. I plan to have a large warning in the release notes, and
perhaps also in the release announcement. It should only take hours on the few
VERY large installations that exist -- on landfill (a very slow machine) it took
about 1-2 minutes to run against the bugzilla-tip database (2400 bugs).

 I'll add a timer, and give people 30 seconds to abort it before we start. Or
perhaps I'll put up a "press any key to continue" prompt that just runs after
two minutes, even with no user feedback. It's much safer that way for people who
run checksetup in a cron. 

  Good idea.

> Particularly if the abortability/continuability of the operation is 
> sub-perfect. (I don't know, I only quickly browsed through the code so far)

  Yeah, the abortability is definitely sub-perfect. We check for the existence
of just one of the old indexes, and then rename all of them. We don't want to
run the checks of all the indexes every time we run checketup -- the code hits
the database with a SHOW INDEXES once for every column in every table in the
database, in addition to whatever magic DBI is doing to get the column lists
themselves.

  After some other changes that I'll make in the future, I could probably wrap
it in a better check, which would be a check for "if the bz_schema table is
empty." Maybe I could just wrap it in that check now... but it will always run
until I actually check in all the code from bug 285111, which will actually
populate the bz_schema table.

Comment 7

[Max Kanat-Alexander]

Attachment: v2

OK, I've added the little warning. That was a good idea. :-)

Comment 8

[Jouni Heikniemi]

Comment on attachment 178348 [details] [diff] [review]
v2

When I first ran the patch, I got the following:

--
DBD::mysql::db do failed: BLOB/TEXT column 'short_desc' used in key
specification without a key length at Bugzilla/DB/Mysql.pm line 335
       
Bugzilla::DB::Mysql::bz_setup_database('Bugzilla::DB::Mysql=HASH(0x97d93e0)')
called at checksetup.pl line 1633
--

On the next run, I got:

--
Adding full-text index for short_desc column in bugs table...
--

After that, everything seemed to work pretty fine (but of course, there's no
easy way to verify the schema was actually correct after everything was done).


>Index: Bugzilla/DB.pm
>===================================================================

>+# Only Bugzilla::DB and subclases should use these methods.

Nit: subclasSes

>+# Gets a list of the existing tables (if any) in the on-disk database.
>+sub bz_table_list_real {

Nit: Add a comment about the return type of the method here.

>Index: Bugzilla/DB/Mysql.pm
>===================================================================

>+            flaginclusions => {
>+                type_id => 'flaginclusions_tpcid_idx'
>+            },
>+            flagexclusions => {
>+                type_id => 'flagexclusions_tpc_id_idx'

Why the difference with the underscores?

>+                    elsif ($index->{TYPE} eq 'UNIQUE') {
>+                        $new_name = $table . "_unique_idx";
>+                    } 

... so we can only have one unique index per table? Why?

>+=item C<bz_index_info_real($table, $index)>
>+
>+ Description: Returns information about an index on a table in the database.
>+ Params:      $table = name of table containing the index (scalar)

Nit: Remove the (scalar) unless you find it particularly necessary - names tend
to be scalar by definition, but the "(scalar)" can be understood as referring
to the index, which is more confusing a concept. Not that it would mean
anything sensible though...


The code looks nice and I almost understand it thoroughly ;-) Since I can't
tell if the error mentioned first was caused by a DB anomaly of mine, I'll r-
until the reasons have been uncovered. I have a working backup on the DB prior
to the index conversion so I can try to repeat the problem if you have an idea
on how to test this further.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> DBD::mysql::db do failed: BLOB/TEXT column 'short_desc' used in key
> specification without a key length at Bugzilla/DB/Mysql.pm line 335

  Hrm, that could have to do with a bug in bz_index_info_real. I noticed that
some of the info it returns is slightly different on MySQL 4.1, but I'm not
certain... what version of MySQL were you testing on?

> After that, everything seemed to work pretty fine (but of course, there's no
> easy way to verify the schema was actually correct after everything was done).

  Hrm, yeah. You can do a "SHOW INDEX FROM bugs" as a simple test to see if they
got renamed.

> Nit: subclasSes

  Thanks, will fix.

> Nit: Add a comment about the return type of the method here.

  OK, will do. :-)

> Why the difference with the underscores?

  These indexes are being renamed to how they are currently named in
Bugzilla::DB::Schema. They are named in strange ways because Schema was an
urgent, huge patch and I didn't go over all the index names in detail to make
sure that they were consistent. That's why this whole special_names thing exists
in the first place.

  However, if I messed up and I'm not renaming them to how they're named in
Schema, do let me know that.

> ... so we can only have one unique index per table? Why?

  Also a bad decision made when naming indexes in Bugzilla::DB::Schema.

> Nit: Remove the (scalar) unless you find it particularly necessary

  Will do.

> I have a working backup on the DB prior
> to the index conversion so I can try to repeat the problem if you have an idea
> on how to test this further.

  Yeah, attach the before-and-after output of "SHOW INDEX FROM bugs". (It won't
fit in a comment; the table will wrap.)

Comment 10

[Jouni Heikniemi]

Attachment: Jouni's show index results for patch v2

(In reply to comment #9)
>   Hrm, that could have to do with a bug in bz_index_info_real. I noticed that

> some of the info it returns is slightly different on MySQL 4.1, but I'm not
> certain... what version of MySQL were you testing on?

mysql  Ver 14.7 Distrib 4.1.8, for pc-linux-gnu (i686)

> Yeah, attach the before-and-after output of "SHOW INDEX FROM bugs". (It won't

> fit in a comment; the table will wrap.)

There you go.

Comment 11

[Max Kanat-Alexander]

Attachment: v3

OK, thanks for that output. That totally explains the problem. :-) Thanks for
catching that, too -- that would have been *very* difficult to fix after
checkin.

From now on I'll definitely test all these _real functions on both MySQL 3 and
4, if they use manual schema-inspection stuff instead of DBI. :-)

Anyhow, this one should work, and should actually successfully rename all the
indexes. :-)

Comment 12

[Max Kanat-Alexander]

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

OK, I've discovered some problems upon further testing.

We need to deal with some old tables that had two unique indexes (or just deal
here with the fact that having only one unique index per table is stupid).

We also need to modify checksetup to use an index name on all the old CREATE
INDEX statements.

Comment 13

[Max Kanat-Alexander]

Attachment: v4

OK, this patch just got a WHOLE lot larger.

Basically, we fully fix all index naming inconsistencies now, and we also make
sure that checksetup uses these names.

I have a test-checksetup script that tells me the raw schema differences
between my patch and the tip (using some cleverness with mysqldump and diff),
and I've used it to confirm that I'm renaming the indexes properly and can
upgrade from: 2.8, 2.14, 2.16, and 2.18.

It can also run against the tip properly.

Comment 14

[Jouni Heikniemi]

Comment on attachment 178732 [details] [diff] [review]
v4

Max, next time you most something of this size, please spend the two minutes it
takes to write a short description on the changes you've made. For example, you
could say, "Most of the checksetup.pl changes are related to reshaping the
index creation clauses for changes made before the appearance of the new schema
system" or whatever.


r=jouni; It looks good and works for me. However, I admit I don't have
sufficient knowledge in all the new DB code to be absolutely certain it'll work
under all circumstances. Depending on the amount of your own testing, you might
want to ask for second review.

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> (From update of attachment 178732 [details] [diff] [review] [edit])
> Max, next time you most something of this size, please spend the two minutes 
> it takes to write a short description on the changes you've made. [snip]

  Yeah, my apologies on that. :-) Will do, next time. And you can always feel
free to email me and ask me for some more details, or just leave a comment on
the bug and postpone the review until I get you the info you need.

> Depending on the amount of your own testing, you might want to ask for second 
> review.

  I did test it pretty thoroughly; I have control over the checksetup tinderbox
script, and I can run it on my own against my patches. So I think the code works
pretty well.

  It does need to be *slightly* updated to deal with one mistake made in emailprefs.

Comment 16

[Max Kanat-Alexander]

OK, I'm about to upload another patch to this bug that fixes two mistakes in the
emailprefs patch:

  (1) The indexes started with email_settings_ even though the table was called
email_setting.

  (2) Having a multi-column index that starts with user_id and having an index
that contains only user_id is *totally redundant*. MySQL will use the first
column of a multi-column index as though it was a normal index on that field. I
believe that most DBs are capable of this, in fact.

Comment 17

[Max Kanat-Alexander]

Attachment: v5

OK, the only changes here are to Schema on email_setting, and a bit of
migration code inside of Mysql.pm's bz_setup_database to fix the email_setting
stuff.

I'd love to just carry forward r+, because I know this works (I tested it, the
testing is simple), but it's some actual code change, so I just need a quick
head-bob on it. :-)

Comment 18

[Jouni Heikniemi]

Comment on attachment 179472 [details] [diff] [review]
v5

r=jouni based on metadiff inspection and a quick test run.

Comment 19

[Max Kanat-Alexander]

OK, I decided that it's actually OK to check this in without a published
relnote, because no really gigantic installation will be using 2.19.3.

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.385; previous revision: 1.384
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.19; previous revision: 1.18
done

Comment 20

[Max Kanat-Alexander]

I had to change one line in checksetup after I checked this in. (The new
schema-checking test-checksetup informed me of my error):

From

$dbh->do("CREATE UNIQUE INDEX component_product_id_idx"

To

$dbh->do("CREATE UNIQUE INDEX components_product_id_idx"

I got approval from justdave for the change.

Comment 21

[Max Kanat-Alexander]

Relnote added to 2.20 relnotes about the fact that checksetup may take a while
to run.