From: Date: June 17 2008 2:39pm Subject: bzr commit into mysql-6.0-backup branch (cbell:2618) Bug#36749 List-Archive: http://lists.mysql.com/commits/47996 X-Bug: 36749 Message-Id: <200806171239.m5HCdqeE006959@mail.mysql.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit #At file:///C:/source/bzr/mysql-6.0-bug-36749/ 2618 Chuck Bell 2008-06-17 BUG#36749 Data Loss after Restore, if Trigger fired on the table that is being Restored. The MyISAM native driver is not locking its tables. This results in data losson restore as either a collision or an overwrite condition occurs. This patch corrects the problem by taking an exclusive name lock on all tables prior to restore of the data (after the metadata step). added: mysql-test/r/backup_lock_myisam.result mysql-test/t/backup_lock_myisam.test modified: sql/backup/data_backup.cc sql/backup/image_info.h sql/share/errmsg.txt sql/si_objects.cc sql/si_objects.h per-file messages: mysql-test/r/backup_lock_myisam.result New result file. mysql-test/t/backup_lock_myisam.test New test for MyISAM locking problem. sql/backup/data_backup.cc Added code to obtain name locks on all tables prior to restore and release them after restore. Code includes method to gather tables from all snapshots. sql/backup/image_info.h Added method to return list of tables from snapshot. sql/share/errmsg.txt New error message for name locking errors. sql/si_objects.cc Added code to obtain and release name locks. sql/si_objects.h New method primitives for obtaining and releasing name locks. === added file 'mysql-test/r/backup_lock_myisam.result' --- a/mysql-test/r/backup_lock_myisam.result 1970-01-01 00:00:00 +0000 +++ b/mysql-test/r/backup_lock_myisam.result 2008-06-17 12:40:00 +0000 @@ -0,0 +1,92 @@ +SET DEBUG_SYNC= 'RESET'; +From con1: +DROP DATABASE IF EXISTS db1; +DROP DATABASE IF EXISTS db2; +Create database 1 and a table then populate it +CREATE DATABASE db1; +CREATE TABLE db1.t1 (a INT) ENGINE=MYISAM; +INSERT INTO db1.t1 VALUES (1),(2),(3),(4),(5),(6),(7),(8),(9),(0); +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +CREATE TABLE db1.t2 (a int) ENGINE=MEMORY; +INSERT INTO db1.t2 VALUES (1),(2),(3),(4),(5); +CREATE TABLE db1.t3 (a int) ENGINE=INNODB; +INSERT INTO db1.t3 VALUES (11),(12),(13); +Show initial count of table +SELECT COUNT(*) FROM db1.t1; +COUNT(*) +327680 +SELECT COUNT(*) FROM db1.t2; +COUNT(*) +5 +SELECT COUNT(*) FROM db1.t3; +COUNT(*) +3 +From con2: +Create database 2 and a table then populate it and add a trigger +that updates the table in database 1 +CREATE DATABASE db2; +CREATE TABLE db2.t2 (A INT); +CREATE TRIGGER db2.trg AFTER INSERT ON db2.t2 FOR EACH ROW +BEGIN +INSERT INTO db1.t1 VALUES ('99'); +END| +From con1: +Now do the backup +BACKUP DATABASE db1 TO 'db1.bak'; +backup_id +# +DROP DATABASE db1; +now start the restore and while the restore is running, fire the trigger +activate synchronization points for restore. +SET DEBUG_SYNC= 'restore_in_progress SIGNAL wait_for_restore WAIT_FOR finish'; +RESTORE FROM 'db1.bak'; +From breakpoints: +Wait for restore to reach its synchronization point. +SET DEBUG_SYNC= 'now WAIT_FOR wait_for_restore'; +breakpoints: Show process list. +SELECT id, command, state, info FROM INFORMATION_SCHEMA.PROCESSLIST +WHERE info LIKE "RESTORE%"; +From con2: +Now do the insert while restore is running. +INSERT INTO db2.t2 VALUES (0); +From breakpoints: +breakpoints: Sending finish signal to wake restore. +SET DEBUG_SYNC= 'now SIGNAL finish'; +Reattach to connection 2 and finish. +Reattach to connection 1 and finish. +backup_id +# +Show the count for t1. It should be 1 more than before restore. +SELECT COUNT(*) FROM db1.t1; +COUNT(*) +327681 +SELECT * FROM db1.t2; +a +1 +2 +3 +4 +5 +SELECT * FROM db1.t3; +a +11 +12 +13 +cleanup +DROP DATABASE db1; +DROP DATABASE db2; +SET DEBUG_SYNC= 'RESET'; === added file 'mysql-test/t/backup_lock_myisam.test' --- a/mysql-test/t/backup_lock_myisam.test 1970-01-01 00:00:00 +0000 +++ b/mysql-test/t/backup_lock_myisam.test 2008-06-17 12:40:00 +0000 @@ -0,0 +1,142 @@ +# +# This test was created to ensure appropriate locks are obtained on the myisam +# tables during a myisam native driver restore. See BUG#36749. +# +# The test uses two connections and debug synchronization to ensure the restore +# in the middle of processing when a trigger attempts to insert data. +# + +--source include/not_embedded.inc +--source include/have_innodb.inc +--source include/have_debug_sync.inc + +SET DEBUG_SYNC= 'RESET'; + +connect(con1, localhost, root,,); +connect(con2, localhost, root,,); +connect(breakpoints, localhost, root,,); + +--echo From con1: +--connection con1 + +--disable_warnings +DROP DATABASE IF EXISTS db1; +DROP DATABASE IF EXISTS db2; +--enable_warnings + +--error 0, 1 +--remove_file $MYSQLTEST_VARDIR/master-data/db1.bak + +--echo Create database 1 and a table then populate it +CREATE DATABASE db1; +CREATE TABLE db1.t1 (a INT) ENGINE=MYISAM; + +INSERT INTO db1.t1 VALUES (1),(2),(3),(4),(5),(6),(7),(8),(9),(0); +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; + +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; +INSERT INTO db1.t1 SELECT * FROM db1.t1; + +CREATE TABLE db1.t2 (a int) ENGINE=MEMORY; + +INSERT INTO db1.t2 VALUES (1),(2),(3),(4),(5); + +CREATE TABLE db1.t3 (a int) ENGINE=INNODB; + +INSERT INTO db1.t3 VALUES (11),(12),(13); + +--echo Show initial count of table +SELECT COUNT(*) FROM db1.t1; + +SELECT COUNT(*) FROM db1.t2; + +SELECT COUNT(*) FROM db1.t3; + +--echo From con2: +--connection con2 + +--echo Create database 2 and a table then populate it and add a trigger +--echo that updates the table in database 1 + +CREATE DATABASE db2; +CREATE TABLE db2.t2 (A INT); + +DELIMITER |; + +CREATE TRIGGER db2.trg AFTER INSERT ON db2.t2 FOR EACH ROW +BEGIN + INSERT INTO db1.t1 VALUES ('99'); +END| + +delimiter ;| + +--echo From con1: +--connection con1 + +--echo Now do the backup +--replace_column 1 # +BACKUP DATABASE db1 TO 'db1.bak'; + +DROP DATABASE db1; + +--echo now start the restore and while the restore is running, fire the trigger +--echo activate synchronization points for restore. +SET DEBUG_SYNC= 'restore_in_progress SIGNAL wait_for_restore WAIT_FOR finish'; +--send RESTORE FROM 'db1.bak' + +--echo From breakpoints: +--connection breakpoints +--echo Wait for restore to reach its synchronization point. +SET DEBUG_SYNC= 'now WAIT_FOR wait_for_restore'; + +--echo breakpoints: Show process list. +--replace_column 1 # +query_vertical SELECT id, command, state, info FROM INFORMATION_SCHEMA.PROCESSLIST +WHERE info LIKE "RESTORE%"; + +--echo From con2: +--connection con2 +--echo Now do the insert while restore is running. +send INSERT INTO db2.t2 VALUES (0); + +--echo From breakpoints: +--connection breakpoints +--echo breakpoints: Sending finish signal to wake restore. +SET DEBUG_SYNC= 'now SIGNAL finish'; + +--echo Reattach to connection 2 and finish. +--connection con2 +--reap + +--echo Reattach to connection 1 and finish. +--connection con1 +--replace_column 1 # +--reap + +--echo Show the count for t1. It should be 1 more than before restore. +SELECT COUNT(*) FROM db1.t1; + +SELECT * FROM db1.t2; + +SELECT * FROM db1.t3; + +--echo cleanup +DROP DATABASE db1; +DROP DATABASE db2; + +SET DEBUG_SYNC= 'RESET'; + +--error 0, 1 +--remove_file $MYSQLTEST_VARDIR/master-data/db1.bak === modified file 'sql/backup/data_backup.cc' --- a/sql/backup/data_backup.cc 2008-05-05 15:03:24 +0000 +++ b/sql/backup/data_backup.cc 2008-06-17 12:40:00 +0000 @@ -342,6 +342,35 @@ int get_default_snapshot_tables(backup:: DBUG_RETURN(0); } +/* + Collect tables from all drivers for taking name lock on the tables. +*/ +int get_all_snapshot_tables(Table_list *t, + TABLE_LIST **tables, + TABLE_LIST **tables_last) +{ + TABLE_LIST *table_list= *tables; + TABLE_LIST *table_list_last= *tables_last; + + DBUG_ENTER("backup::get_all_snapshot_tables"); + /* + If the table list is empty, use the first one and loop + until the end then record the end of the first one. + */ + if (!table_list) + { + table_list= build_table_list(*t, TL_WRITE); + *tables= table_list; + table_list_last= table_list; + while (table_list_last->next_global != NULL) + table_list_last= table_list_last->next_global; + *tables_last= table_list_last; + } + else + (*tables_last)->next_global= build_table_list(*t, TL_WRITE); + DBUG_RETURN(0); +} + /** Commit Blocker @@ -1364,7 +1393,7 @@ namespace backup { /** Read backup image data from a backup stream and forward it to restore drivers. */ -int restore_table_data(THD*, Restore_info &info, Input_stream &s) +int restore_table_data(THD *thd, Restore_info &info, Input_stream &s) { DBUG_ENTER("restore::restore_table_data"); @@ -1377,6 +1406,8 @@ int restore_table_data(THD*, Restore_inf TABLE_LIST *table_list= 0; TABLE_LIST *table_list_last= 0; + TABLE_LIST *all_tables= 0; + TABLE_LIST *all_tables_last= 0; if (info.snap_count() > 256) { @@ -1412,6 +1443,11 @@ int restore_table_data(THD*, Restore_inf (snap->type() == Snapshot_info::CS_SNAPSHOT)) get_default_snapshot_tables(NULL, (default_backup::Restore *)drv[n], &table_list, &table_list_last); + /* + Collect tables from all drivers for name locking. + */ + get_all_snapshot_tables((Table_list *)snap->get_table_list(), + &all_tables, &all_tables_last); } /* @@ -1430,6 +1466,15 @@ int restore_table_data(THD*, Restore_inf table_list_last->next_global= NULL; // break lists } + /* + Apply name locks to all tables used. + */ + if (obs::get_name_locks(thd, all_tables, TRUE)) + { + info.m_ctx.fatal_error(ER_BACKUP_RESTORE_NAME_LOCK_FAILED, "obtain"); + goto error; + } + // Initialize the drivers. for (uint n=0; n < info.snap_count(); ++n) { @@ -1441,6 +1486,7 @@ int restore_table_data(THD*, Restore_inf } } + DEBUG_SYNC(thd, "restore_in_progress"); { Buffer buf; uint snap_num=0; @@ -1590,6 +1636,12 @@ int restore_table_data(THD*, Restore_inf if (!bad_drivers.is_empty()) info.m_ctx.report_error(ER_BACKUP_STOP_RESTORE_DRIVERS, bad_drivers.c_ptr()); } + + /* + Release name locks on driver tables. + */ + if (obs::release_name_locks(thd, all_tables)) + info.m_ctx.fatal_error(ER_BACKUP_RESTORE_NAME_LOCK_FAILED, "release"); /* Close all tables if default or snapshot driver used. === modified file 'sql/backup/image_info.h' --- a/sql/backup/image_info.h 2008-05-05 15:06:40 +0000 +++ b/sql/backup/image_info.h 2008-06-17 12:40:00 +0000 @@ -261,6 +261,8 @@ class Snapshot_info virtual ~Snapshot_info(); + Image_info::Tables *get_table_list() { return &m_tables; } + protected: version_t m_version; ///< Stores version number of the snapshot's format. === modified file 'sql/share/errmsg.txt' --- a/sql/share/errmsg.txt 2008-05-05 19:38:05 +0000 +++ b/sql/share/errmsg.txt 2008-06-17 12:40:00 +0000 @@ -6357,3 +6357,6 @@ ER_DEBUG_SYNC_HIT_LIMIT eng "debug sync point hit limit reached" ger "Debug Sync Point Hit Limit erreicht" +ER_BACKUP_RESTORE_NAME_LOCK_FAILED + eng "Restore failed to %-64s the name locks on the tables." + === modified file 'sql/si_objects.cc' --- a/sql/si_objects.cc 2008-05-05 20:19:03 +0000 +++ b/sql/si_objects.cc 2008-06-17 12:40:00 +0000 @@ -3071,6 +3071,64 @@ void ddl_blocker_exception_off(THD *thd) DBUG_VOID_RETURN; } +/** + Gets name locks on table list. + + This method attempts to take a name lock on each table in the list. + The lock can be exclusive or not. It does nothing if the table list is + empty. + + @param[IN] thd current thread + @param[IN] tables List of tables to take name locks + @param[IN] exclusive If true, the operation takes and exclusive name lock + + @returns 0 if success, 1 if error +*/ +int get_name_locks(THD *thd, TABLE_LIST *tables, bool exclusive) +{ + int ret= 0; + DBUG_ENTER("obs::get_name_locks()"); + if (tables) + { + pthread_mutex_lock(&LOCK_open); + if (exclusive) + { + if (lock_table_names_exclusively(thd, tables)) + ret= 1; + } + else + { + if (lock_table_names(thd, tables)) + ret= 1; + } + pthread_mutex_unlock(&LOCK_open); + } + DBUG_RETURN(ret); +} + +/* + Releases name locks on table list. + + This method releases the name locks on the table list. It does nothing if + the table list is empty. + + @param[IN] thd current thread + @param[IN] tables List of tables to take name locks + + @returns 0 if success, 1 if error +*/ +int release_name_locks(THD *thd, TABLE_LIST *tables) +{ + DBUG_ENTER("obs::release_name_locks()"); + if (tables) + { + pthread_mutex_lock(&LOCK_open); + unlock_table_names(thd, tables, (TABLE_LIST*) 0); + pthread_mutex_unlock(&LOCK_open); + } + DBUG_RETURN(0); +} + } // obs namespace /////////////////////////////////////////////////////////////////////////// === modified file 'sql/si_objects.h' --- a/sql/si_objects.h 2008-05-05 15:03:24 +0000 +++ b/sql/si_objects.h 2008-06-17 12:40:00 +0000 @@ -604,6 +604,16 @@ COND *create_db_select_condition(THD *th TABLE *t, List *db_list); +/* + Gets name locks on table list. +*/ +int get_name_locks(THD *thd, TABLE_LIST *tables, bool exclusive); + +/* + Releases name locks on table list. +*/ +int release_name_locks(THD *thd, TABLE_LIST *tables); + } // obs namespace #endif // SI_OBJECTS_H_