Thanks for the quick review.
Guilhem Bichot wrote:
> Hello Marc,
> Marc Alff a écrit, Le 05.03.2010 16:31:
>> #At file:///home/malff/BZR_TREE/mysql-next-mr-bugfixing-45194/ based
>> on revid:alik@stripped
>> 3122 Marc Alff 2010-03-05
>> Bug#45194 mysql_upgrade deletes existing data in
>> performance_schema database/schema
>> Before this fix, mysql_upgrade would always drop and re
>> the performance_schema database.
>> This in theory could destroy user data created using 5.1 or
>> older versions.
>> With this fix, mysql_upgrade checks the content of the
>> performance_schema database before droping it.
>> === added file 'mysql-test/suite/perfschema/r/pfs_upgrade.result'
>> --- a/mysql-test/suite/perfschema/r/pfs_upgrade.result 1970-01-01
>> 00:00:00 +0000
>> +++ b/mysql-test/suite/perfschema/r/pfs_upgrade.result 2010-03-05
>> 15:31:01 +0000
>> @@ -0,0 +1,153 @@
>> +drop table if exists test.user_table;
>> +drop procedure if exists test.user_proc;
>> +drop function if exists test.user_func;
>> +drop event if exists test.user_event;
>> +"Testing mysql_upgrade with TABLE performance_schema.user_table"
>> +create table test.user_table(a int);
>> +use performance_schema;
>> +show tables like "user_table";
>> +Tables_in_performance_schema (user_table)
>> +ERROR 1644 (HY000) at line 178: Unexpected content found in the
>> performance_schema database.
>> +ERROR 1050 (42S01) at line 203: Table 'COND_INSTANCES' already exists
> So even if we detect the presence of unexpected content, we still try to
> create COND_INSTANCES, and in the end we report a failed upgrade.
> What is the gain of creating COND_INSTANCES in this case...?
None, but at least this does not destroy user data, so this is safe.
I don't want to make the script more complicated that it already is, to
account for "more cosmetic" error reporting.
> I would have imagined that as soon as we see that performance_schema has
> unexpected content, we decide that this database is something unknown,
> possibly unrelated to Performance Schema, which we are not allowed to
> spoil, so we don't create tables there, and report a failed upgrade?
> What do you think?
This is a question for the mysql_upgrade client itself, which despite
having been returned an error code with SIGNAL, still continues to
execute the commands in the upgrade script ...
I don't know if this is by design or not, but mysql_upgrade seems to try
to do as much as possible, and only reports a final failure at the end.
On one hand, this seems more robust, since a minor failure affecting an
area never used does not prevent the upgrade on unrelated areas.
On the other hand, this seems more dangerous, since who knows what can
happen when executing blindly every command without testing the results.
As far as the command issued by the performance schema part of the
scripts are concerned, these commands are safe, and will either pass and
produce the expected result, or fail without causing damage, so the
performance schema script is safe even if mysql_upgrade does not stop on
the first error.
>> === modified file 'scripts/mysql_system_tables.sql'
>> --- a/scripts/mysql_system_tables.sql 2010-01-12 01:47:27 +0000
>> +++ b/scripts/mysql_system_tables.sql 2010-03-05 15:31:01 +0000
>> +set @have_old_pfs= (select count(*) from information_schema.schemata
>> where schema_name='performance_schema');
>> +SET @l1="SET @broken_tables = (select count(*) from
>> +SET @l2=" where engine != \'PERFORMANCE_SCHEMA\' and
>> +SET @cmd=concat(@l1,@l2);
>> +-- Work around for bug#49542
> nice trick :-)
What I like here is that this will work for downgrades, even with more
tables implemented in the P_S in later releases.
But I think the SIGNAL trick is nicer :-)