310325 - Workaround MySQL bug incorrectly thinking a column contains NULL values

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Olav Vitters]

When using bz_alter_column from Bugzilla/DB.pm to make a column NOT NULL the
function checks if there are currently NULL values in that column. MySQL 4.1.14
(current latest stable) has a bug that will *incorrectly* think there *are* NULL
values in that column. This causes bz_alter_column to die(). The MySQL bug has
been verified by a MySQL developer, see:
  http://bugs.mysql.com/bug.php?id=13535

Note: 5.0 versions are safe, but those are in release-candidate stage. Not
having this workaround would require those 5.0 versions.

The bug only occurs the first time the query is ran. The second time will return
the correct result. As this happens in the latest MySQL stable and the
workaround is easy, Bugzilla should workaround this bug (rather than waiting for
a MySQL with this bug fixed). Impact is low as bz_alter_column is only called by
checksetup.pl.


To reproduce:
1. DROP DATABASE bugs
2. CREATE DATABASE bugs
3. ./checksetup.pl
4. Login with admin account
5. Apply patch from bug 119524 (attachment 197620 [details] [diff] [review])
6. ./checketup.pl

Without a workaround and with MySQL 4.1.14, this will incorrectly fail on the IS
NULL check. I have a new patch for that bug, but that one will still fail on the
IS NULL check.

Comment 1

[Olav Vitters]

Attachment: Patch v1: Workaround bug by running the query twice under MySQL (Bugzilla HEAD 27 Sep 2005)

Patch will make Bugzilla run the query twice under MySQL. Tested this with the
attachment from bug 119524. It correctly works around the bug. See steps to
reproduce to test it. Note that due to bug 310231 you will get a MySQL error
about the PRIMARY KEY being added twice (ehr... when I add the new patch in bug
119524). This is ok, the SQL causing that is executed after the IS NULL check.
Perhaps you want to apply the patch from bug 310231 first though.

Comment 2

[Olav Vitters]

A patch has been posted at the MySQL bugtracker. Cause was a bug in the 'SHOW
TABLE STATUS'. Bugzilla calls this at Bugzilla/DB/Mysql.pm, bz_setup_database.

Quoting from the patch at http://lists.mysql.com/internals/30652:
>  After SHOW TABLE STATUS last_insert_id wasn't cleaned, and next select
>  erroneously rewrites WHERE condition and returs a row;
>  5.0 isn't affected because of different SHOW TABLE STATUS handling.
>  
> last_insert_id cleanup added to mysqld_extend_show_tables().

Seems pretty bad. Reading this, it could impact any select statement. Now we
know the real cause I'm going to test if I can make a patch that'll just do
something like SELECT 1 WHERE 1=0 (not sure if this will work around it or if I
need something that has 'FROM logincookies').

Comment 3

[Olav Vitters]

(In reply to comment #2)
> Seems pretty bad. Reading this, it could impact any select statement. Now we
> know the real cause I'm going to test if I can make a patch that'll just do
> something like SELECT 1 WHERE 1=0 (not sure if this will work around it or if I
> need something that has 'FROM logincookies').

Tried various workarounds, but only repeating the select statement (as in the
patch) works reliably.

Comment 4

[Olav Vitters]

Bug will be fixed in MySQL 4.1.16. Seems it was too late to put into 4.1.15
(isn't out yet).

Comment 5

[Olav Vitters]

Comment on attachment 197721 [details] [diff] [review]
Patch v1: Workaround bug by running the query twice under MySQL (Bugzilla HEAD 27 Sep 2005)

This one works around a bug in MySQL which results in Bugzilla thinking the
columns contains NULL values while it does not. Instead of fixing this we could
wait for MySQL (unreleased) 4.1.16 (has the fix), but as 4.1.15 isn't even
released yet + patch only does the workaround (running the query twice) for
MySQL, I think this should go in.

First comment says how to test for the bug. I've used at least MySQL 4.1.12 and
.14 to reproduce this.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 197721 [details] [diff] [review]
Patch v1: Workaround bug by running the query twice under MySQL (Bugzilla HEAD 27 Sep 2005)

Instead of the isa check (which I really don't like having, in parent modules), just always run the check twice. It couldn't hurt, and it's not like it's slow.

Comment 7

[Bradley Baetz (:bbaetz)]

Does this affect 4.0 as well? And is it all of 4.1, or just some? We have blacklisted mysql versions before.

Although this is only checksetup, so....

Maybe an alternative is to run a dummy select of some sort after SHOW TABLE STATUS? From the bug it seems that the first query will be wrong - future changes to checksetup and related code could change which query is broken.

Comment 8

[Max Kanat-Alexander]

It's all of MySQL before 4.1.17, I believe. I think 3.x isn't affected.

But yeah, I agree with Bradley that we should put a dummy select after whatever does SHOW TABLE STATUS (probably a table_info or column_info call).

Comment 9

[Olav Vitters]

(In reply to comment #8)
> It's all of MySQL before 4.1.17, I believe. I think 3.x isn't affected.

Will be fixed in the unreleased 4.1.16, it is already in the changelog on http://dev.mysql.com/

> But yeah, I agree with Bradley that we should put a dummy select after whatever
> does SHOW TABLE STATUS (probably a table_info or column_info call).

I tried after creating the patch, but couldn't figure out an SQL to add. Adding the specific 'IS NULL' SQL seemed hackish. Will try table_info & column_info to see if they help.

(In reply to comment #7)
> Does this affect 4.0 as well? And is it all of 4.1, or just some? We have
> blacklisted mysql versions before.

I'm not 100% sure, but thought 4.1.7 had the bug as well, but didn't try any versions before that.

Ok, just installed MySQL 4.0.26 and tried that: works. If blacklist is a better option then I'd suggest blacklisting 4.1.0 - 4.1.15.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> I tried after creating the patch, but couldn't figure out an SQL to add. 
> Adding the specific 'IS NULL' SQL seemed hackish. 

  That would be OK. A workaround is a hack by definition.

> Will try table_info & column_info to see if they help.

  I meant that it's probably table_info or column_info that's doing the SHOW TABLE STATUS.

> Ok, just installed MySQL 4.0.26 and tried that: works. If blacklist is a 
> better option then I'd suggest blacklisting 4.1.0 - 4.1.15.

  Yeah, that would be too broad, that would exclude most of our users. :-)

Comment 11

[Max Kanat-Alexander]

Attachment: Add a dummy SELECT after the only SHOW TABLE STATUS

As far as I can tell, there's only one SHOW TABLE STATUS in all of Bugzilla. Even table_info and column_info just use SHOW TABLES. I examined the MySQL patch, I think that SHOW TABLES is unaffected.

I don't know if this patch works, though. Olav, could you test it and let me know?

Comment 12

[Olav Vitters]

Comment on attachment 206007 [details] [diff] [review]
Add a dummy SELECT after the only SHOW TABLE STATUS

I tried that exact SQL after you suggested I should try other SQLs. Applied patch and tried again. Doesn't fix the problem. Tried putting the SQL used in the conversion ('SELECT 1 FROM logincookies WHERE cookie IS NULL') and that does work. Also tried the query  'SELECT 1 FROM bugs WHERE rep_platform IS NULL'. That one does NOT work (tried with empty bugs table and after creating a bug). Also tried 'SELECT 1 FROM logincookies WHERE 0'; does NOT work.

Currently to reproduce this (beside downgrading MySQL) I have to avoid using latest CVS (I use CVS from 2005-09-28 to test this patch). Something between when I saw the problem and when I tried to work on it made my steps-to-reproduce work. Perhaps the SQL avoiding this is soon enough to avoid other problems, but I'm not sure...

(some time later after manually adding each change and testing again)
Ok.. found what avoids the problem the last time I tried to figure this out. The code that changes 'Days since bug changed' (added to checksetup.pl by LpSolit in bug 302599) avoids it... but only if the column is actually converted... if it doesn't have to convert the 'Days since bug changed' fielddef it will still fail the IS NULL check... :-(

Found another SQL that avoids the problem:
  SELECT 1 FROM bugs WHERE bug_id IS NULL

(perhaps it must be a primary key or something?)

Comment 13

[Max Kanat-Alexander]

Yeah, it's whatever will reset LAST_INSERT_ID(), I think according to the MySQL bug.

Just remember that you can't do a query on a table before the table exists.

So, any ideas for a new patch? I'd like to get the dependency into 2.22.

Comment 14

[Olav Vitters]

Attachment: Patch v3

(In reply to comment #13)
> Yeah, it's whatever will reset LAST_INSERT_ID(), I think according to the MySQL
> bug.
> 
> Just remember that you can't do a query on a table before the table exists.
> 
> So, any ideas for a new patch? I'd like to get the dependency into 2.22.
 
Many thanks for that LAST_INSERT_ID comment. I verified (tested) that it only occurs when SHOW TABLE STATUS shows a table with an auto_increment.

If you use SHOW TABLE STATUS on a table with an auto_increment the LAST_INSERT_ID will be 1 and the bug occurs. After using a good workaround the LAST_INSERT_ID is 0 again. 

To test that the bug only occurs with auto_increment tables I tried it on a non-auto_increment table. Example:
>  SHOW TABLE STATUS LIKE 'cc'

After above statement the bug also does not appear and LAST_INSERT_ID is 0, tested using (test requires at least one bug in the table):
>  SELECT LAST_INSERT_ID()
>  SELECT 1 FROM bugs WHERE bug_id IS NULL

(also tested with an auto_increment table)

This means that when there are no tables the bug will not occur (to be sure I tested this as well by looking at LAST_INSERT_ID). So the workaround only is needed when at least one table exist with an auto_increment.

What the patch does is check if the bugs table exists, then check if the bug_id column exists using bz_column_info_real.

I needed to verify the bugs table existed because:
* if the table does not exist the bz_column_info_real will give an error

I used bz_column_info_real because:
* it used directly after below
* other one assumes the bz_schema table exists. It will give an error if the database is empty

Comment 15

[Max Kanat-Alexander]

Comment on attachment 206195 [details] [diff] [review]
Patch v3

Yeah, that looks right. I'm slightly worried about calling the variable @tables -- we might re-use that accidentally. But other than that it's totally good. :-)

Comment 16

[Frédéric Buclin]

Comment on attachment 206195 [details] [diff] [review]
Patch v3

>+        $self->do('SELECT 1 FROM bugs WHERE bug_id IS NULL');

Is it really OK to have a $dbh->do() together with a SELECT? I thought $dbh->do() could only be used with either INSERT, UPDATE or DELETE?

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
> Is it really OK to have a $dbh->do() together with a SELECT? I thought
> $dbh->do() could only be used with either INSERT, UPDATE or DELETE?

  Yes, it's fine. It just discards the result.

Comment 18

[Max Kanat-Alexander]

Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.32; previous revision: 1.31
done

Comment 19

[Olav Vitters]

Attachment: Backport for 2.20

Needed to backport bug 119524 to the 2.20 branch (requested by justdave in bug 119524 comment 23). This patch is the same as the review+ one, except it adds a 'use Bugzilla::Util;'. This was added to Bugzilla/DB/Mysql.pm by a patch in bug 232612. Patch has been tested (of course).

The review+ Bugzilla CVS patches from bug 119524 and bug 310231 apply and work for 2.20 branch.

Comment 20

[Olav Vitters]

LpSolit bkor: keeping the bug closed let it outside our radar
LpSolit for checkins that is
bkor it is a 2.20 backport, reopen it?
LpSolit bkor: at least we would still see it as a bug which has pending checkins

Comment 21

[Max Kanat-Alexander]

Since we have decided not to backport bug 119524, this doesn't need to be backported either.