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...?

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

Welcome to the land of Bugzilla::DB. :-D The patch pretty much explains itself in comments.

(In reply to comment #1) > RENAME INDEX <old_index_name> [ON <table_name>] TO <new_index_name> That only works for MaxDB, unfortunately.

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

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)

(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.

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

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.

(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.)

(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.

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 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.

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 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.

(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.

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.

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 on attachment 179472 [details] [diff] [review] v5 r=jouni based on metadiff inspection and a quick test run.

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

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.

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