451735 - Wrong index for the series table

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Frédéric Buclin]

Currently, the series table has the following index:

    series_creator_idx  =>
        {FIELDS => [qw(creator category subcategory name)],
         TYPE => 'UNIQUE'},

But by looking at the code in Series.pm and chart.cgi, it appears that $series->existsInDatabase only makes sure that the triplet qw(category subcategory name) already exists, i.e. we don't allow another user to create exactly the same triplet. As this seems to be the desired behavior, and I understand why this behavior is the one we want, we should enforce this in the DB itself by fixing the index to:

    series_creator_idx  =>
        {FIELDS => [qw(category subcategory name)],
         TYPE => 'UNIQUE'},

Else if we get a duplicated entry, we would severely be in trouble.

Comment 1

[Frédéric Buclin]

Probably we should have:

    series_creator_idx  => ['creator'],
    series_category_idx =>
        {FIELDS => [qw(category subcategory name)],
         TYPE => 'UNIQUE'},

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

Comment 3

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
Committed revision 7533.

Comment 4

[Frédéric Buclin]

My patch is causing trouble on MySQL:

Removing index 'series_creator_idx' from the series table...
Failed SQL: [DROP INDEX `series_creator_idx` ON series] Error: DBD::mysql::db do failed: Error on rename of './bugs_cvs/#sql-c1c_5' to './bugs_cvs/series' (errno: 150) [for Statement "DROP INDEX `series_creator_idx` ON series"] at Bugzilla/DB.pm line 967
        eval {...} called at Bugzilla/DB.pm line 967
        Bugzilla::DB::bz_drop_index_raw('Bugzilla::DB::Mysql=HASH(0xbea5ca0)', 'series', 'series_creator_idx') called at Bugzilla/DB.pm line 936
        Bugzilla::DB::bz_drop_index('Bugzilla::DB::Mysql=HASH(0xbea5ca0)', 'series', 'series_creator_idx') called at Bugzilla/Install/DB.pm line 3464
        Bugzilla::Install::DB::_fix_series_indexes() called at Bugzilla/Install/DB.pm line 645
        Bugzilla::Install::DB::update_table_definitions('HASH(0xa3caca8)') called at ./checksetup.pl line 199
Adding new index 'series_creator_idx' to the series table ...
DBD::mysql::db do failed: Duplicate key name 'series_creator_idx' [for Statement "CREATE INDEX `series_creator_idx` ON series (creator)"] at Bugzilla/DB.pm line 752
        Bugzilla::DB::bz_add_index_raw('Bugzilla::DB::Mysql=HASH(0xbea5ca0)', 'series', 'series_creator_idx', 'ARRAY(0xbee6900)') called at Bugzilla/DB.pm line 725
        Bugzilla::DB::bz_add_index('Bugzilla::DB::Mysql=HASH(0xbea5ca0)', 'series', 'series_creator_idx', 'ARRAY(0xbee6900)') called at Bugzilla/Install/DB.pm line 3465
        Bugzilla::Install::DB::_fix_series_indexes() called at Bugzilla/Install/DB.pm line 645
        Bugzilla::Install::DB::update_table_definitions('HASH(0xa3caca8)') called at ./checksetup.pl line 199


My guess is that you cannot remove an index on a column which is involved in a FK. This would mean that the FK has to be removed first, and readded only when the new index is in place.

Comment 5

[Frédéric Buclin]

http://dev.mysql.com/doc/refman/5.1/en/innodb-foreign-key-constraints.html

"InnoDB requires indexes on foreign keys and referenced keys so that foreign key checks can be fast and not require a table scan. In the referencing table, there must be an index where the foreign key columns are listed as the first columns in the same order. Such an index is created on the referencing table automatically if it does not exist. (This is in contrast to some older versions, in which indexes had to be created explicitly or the creation of foreign key constraints would fail.)"

mkanat, should bz_drop_index() automatically remove FKs for us? I think they are re-added automatically by checksetup.pl, based on DB/Schema.pm.

Comment 6

[Max Kanat-Alexander]

Yeah, there should be some way for bz_drop_index (or get_drop_index_sql in Schema) to drop the FKs for us and have checksetup re-created them later. All the methods you'd need for that should already exist.

Comment 7

[Frédéric Buclin]

Attachment: fix bz_drop_index() to first delete related FKs, v1

The fix in Bugzilla/Install/DB.pm is for installations which unfortunately already upgraded to current tip code. Fortunately, this check is done only once as we return earlier if indexes have already been updated/created.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 483326 [details] [diff] [review]
fix bz_drop_index() to first delete related FKs, v1

bz_index_info_real only actually exists on MySQL, so the new Install::DB code should go into bz_setup_database in Bugzilla::DB::Mysql instead.

Comment 9

[Frédéric Buclin]

Attachment: fix bz_drop_index() to first delete related FKs, v2

Comment 10

[Max Kanat-Alexander]

Comment on attachment 483346 [details] [diff] [review]
fix bz_drop_index() to first delete related FKs, v2

Beautiful. Thank you! :-)

Comment 11

[Frédéric Buclin]

I hope the problem is now fixed for real. :)

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/DB.pm
modified Bugzilla/DB/Mysql.pm
Committed revision 7542.

Comment 12

[Frédéric Buclin]

Attachment: better fix, v2.1

Max, I think this fix is a better approach than the previous one: we should call bz_index_info() instead of bz_drop_index(), and only fix the DB if it's actually in an inconsistent state. If the stored DB schema matches the real DB schema, then we shouldn't do anything, and let Bugzilla/Install/DB.pm do its job.

I feel much more confident with this additional fix.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 483430 [details] [diff] [review]
better fix, v2.1

Ah, yeah, I agree, I like this more. :-)

Comment 14

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/DB.pm
modified Bugzilla/DB/Mysql.pm
Committed revision 7546.