Patch 2618 approved with the following minor comments:
* The intention of get_all_snapshot_tables would be clearer if it had
documentation saying something like "Appends tables in t to tables and
makes tables_last point to the end of tables"
* Also not sure if get_all_snapshot_tables is a good name. I would
prefer something along the lines of append_... If the method is renamed,
the doc suggestion above may not be necessary.
* I don't think it's good to internationalize only parts of an error
message. In this patch, the error message you get from
ER_BACKUP_RESTORE_NAME_LOCK_FAILED is internationalized with the
exception of the words "obtain" and "release". In Norwegian, e.g, the
internationalized text would not make sense:
"Restore klarte ikke å obtain navnelås på tabellene" <- meaningless
In my opinion, these two words need to be internationalized as well.
Regards,
Jørgen
Chuck Bell wrote:
> #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<LEX_STRING> *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_
>
>
--
Jørgen Løland
