278148 - checksetup.pl *still* errors when trying to drop index on milestones table

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Shane H. W. Travis]

Two bugs have already been filed on this issue: bug 244756, and bug 277303. 
Unfortunately, while the patches on both were reviewed and approved, neither 
one actually works. :(

With the latest patch from bug 277303 applied, I get the following output from 
checksetup.pl:

Fixing Indexes and Uniqueness.
DBD::mysql::st execute failed: You have an error in your SQL syntax 
near 'PRIMARY' at line 1 at ./checksetup.pl line 2321.
DBD::mysql::st fetchrow_arrayref failed: You have an error in your SQL syntax 
near 'PRIMARY' at line 1 at ./checksetup.pl line 2312.


The reason for this is that DropIndexes makes no allowance for the idea that it 
may ever have to drop a PRIMARY key. To do so requires this syntax:

   ALTER TABLE milestones DROP PRIMARY KEY

Perusal of the code in DropIndexes shows that it can only produce the following 
syntax:

   ALTER TABLE milestones DROP INDEX PRIMARY

There are two approaches to fixing this: either fix DropIndexes so that it 
handles PRIMARY key drops properly, or just add the above line with the correct 
syntax in place of the DropIndexes call. The former is 'better', the latter 
is 'faster'... and at this particular moment, with 2.18 release so near, speed 
may be more important.

Comment 1

[Shane H. W. Travis]

Attachment: Code patch for 2.18 and tip

This one works; I've tested it in an upgrade situation myself.

Not only that, it works silently *whether or not* you've recreated the table
from a mysqldump (giving yourself a PRIMARY key) or if you're still running the
original.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 171076 [details] [diff] [review]
Code patch for 2.18 and tip

Was "product" actually marked as the PRIMARY KEY? If it wasn't this won't work
on MySQL greater than 4.1.2. From the MySQL manual:

"# DROP PRIMARY KEY drops the primary index. (Prior to MySQL 4.1.2, if no
primary index exists, DROP PRIMARY KEY drops the first UNIQUE index in the
table. MySQL marks the first UNIQUE key as the PRIMARY KEY if no PRIMARY KEY
was specified explicitly.) If you add a UNIQUE INDEX or PRIMARY KEY to a table,
it is stored before any non-unique index so that MySQL can detect duplicate
keys as early as possible."

Comment 3

[Max Kanat-Alexander]

Attachment: Modify DropIndexes to properly drop PRIMARY KEY

Travis, since I know that you can reproduce the bug, can you test this patch to
see if it works?

I know that this will have no side effects, to modify DropIndexes to work
properly. DropIndexes always would have failed in this fashion before; we would
have seen a syntax error about dropping PRIMARY KEYs before if this was going
to have any side-effects.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 171091 [details] [diff] [review]
Modify DropIndexes to properly drop PRIMARY KEY

Arrrgggghh, I forgot the table name in the ALTER TABLE.

Comment 5

[Max Kanat-Alexander]

Attachment: Version 3

OK, this patch should work. :-)

Comment 6

[Shane H. W. Travis]

Comment on attachment 171092 [details] [diff] [review]
Version 3

Proven to have worked in my testing environment, which is where this problem
resurfaced after the initial bug report. GJ Max!

Comment 7

[Vlad Dascalu]

Comment on attachment 171091 [details] [diff] [review]
Modify DropIndexes to properly drop PRIMARY KEY

Please don't review your own patches.

Comment 8

[Shane H. W. Travis]

2.18
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.22; previous revision: 1.289.2.21
done

Tip:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.325; previous revision: 1.324
done

Comment 9

[Max Kanat-Alexander]

(In reply to comment #7)
> Please don't review your own patches.

  Actually, Travis was the one who did the review, I was just marking it review-
because it in fact was a bad patch as he pointed out to me. I don't see the harm
or statistical data corruption that might be caused by it... although maybe it
would look like I did a review when I didn't (when counting how many reviews
people were doing), so I can understand that concern.