Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2882) Bug#42895 WL#4844

[Ingo Strüwing]

Hi Rafal,

thanks for the review.

Rafal Somla, 30.10.2009 15:16:

...
> Ingo Struewing wrote:
...
>>  2882 Ingo Struewing    2009-10-28
>>       Bug#42895 - Implement a locking scheme for RESTORE
>>       WL#4844 - Implement a locking scheme for RESTORE
>>             The old RESTORE locking scheme did allow other
>>       sessions to modify the tables in restore in some ways.
>>             RESTORE locking does now work as:
>>       1) take BML
>>       2) restore DDL (DROP/CREATE)
>>       3) LOCK TABLES <> WRITE
>>       4) TRUNCATE tables
>>       5) restore data
>>       6) FLUSH TABLES
>>       7) UNLOCK TABLES
>>             This does implicitly fix:
>>       Bug#40944 (Backup: crash after myisampack) and
>>       Bug#41716 (Backup Error when sending data (for table #1)
>>             to Default/Snapshot restore driver)
> ...
> 
>> === modified file 'mysql-test/suite/backup/t/backup_external.test'
...
>> @@ -123,6 +125,11 @@ BACKUP DATABASE db1 TO 'db1.bak';
>>  DROP DATABASE db1;
>>  
>>  # Perform restore
>> +--echo # RESTORE of a compressed MyISAM table issues two warnings:
>> +--echo # "Table is read only" and "Deadlock found when trying to get
>> lock".
>> +--echo # This results from an internal call of FLUSH TABLES.
> 
> [1] First one is perhaps ok, but second one does not look good. I'm
> thinking if warnings should not be disabled when FLUSH TABLES is called
> from inside RESTORE. But then we might miss some relevant warnings...


Yes, that's what I thought too. When developing the patch before
Øystein's warning fixes were pushed, these warnings were simply dropped.
I checked that the same behavior happened there too. It is unfortunate
that it happens at all. But this results from MyISAM specifics with
compressed tables. In this case we cannot get write locks on them. But
as we want to unlock after flush anyway, we don't need the locks, and
thus can accept the problem. But as you say, it may be good to see other
warnings, if they pop up.

A final solution could be a FLUSH TABLES WITH UNLOCK statement. But I'm
hearing yells, imaging to have proposed it...

...
>> === added file 'mysql-test/suite/backup/t/backup_restore_locking.test'
...
>> +--echo #
>> +--echo # Bug#41716 - Backup Error when sending data (for table #1)
>> +--echo #             to Default/Snapshot restore driver
>> +--echo #
>> +CREATE DATABASE bup_reslock_db1;
>> +USE bup_reslock_db1;
>> +CREATE TABLE t1 (c1 INT, c2 VARCHAR(100), UNIQUE(c1)) ENGINE=MEMORY;
>> +INSERT INTO t1 VALUES (1,"abc");
>> +#
>> +--replace_column 1 #
> 
> [3] Perhaps "--replace_column 1 ###" to be more consistent..


Good. Agree.

...
>> +            --echo #
>> +            --echo # connection con5
>> +            --connect (con5, localhost, root,,)
>> +            SET DEBUG_SYNC= 'now WAIT_FOR lock_wait';
>> +            SELECT state, info FROM INFORMATION_SCHEMA.PROCESSLIST;
> 
> [2] I am afraid of using I_S.PROCESSLIST. Many times it has lead to
> non-determinsitic/sporadic test failures which are hard to handle.
> AFAICT each time we got over these problems we solved them by removing
> such selects from the tests. It would be better (for us) to avoid using
> this as testing mechanism. On the other hand, it is silly not to use
> some server feature which should work.


Right. I prefer to wait until it fails. When running this statement, all
other threads block somewhere, waiting for this one. The selected
columns are expected to give stable results. I'd say it is even good to
have a check that this situation does not change.

> 
>> +            SET DEBUG_SYNC= 'now SIGNAL continue';
>> +            --disconnect con5
>> +#
>> +--echo #
>> +--echo # connection default
> 
> [3] I think it would be nice to print info about which statement is
> going to be reaped.


Good. Agree.

...

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder,   Wolfgang Engels,   Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028