List:Commits« Previous MessageNext Message »
From:Ingo Struewing Date:December 3 2008 4:02pm
Subject:bzr commit into mysql-6.0-backup branch (ingo.struewing:2737) Bug#40944
View as plain text  
#At file:///home2/mydev/bzrroot/mysql-6.0-bug40944-2/

 2737 Ingo Struewing	2008-12-03
      Bug#40944 - Backup: crash after myisampack
      
      After restore of a compressed MyISAM table, the server crashed
      when the table was accessed.
      
      The problem was twofold.
      
      1. The crash happened in the MyISAM keycache. It did not handle
      an error in reading a block correctly. This is now fixed in
      key_cache_read(), key_cache_insert(), and key_cache_write().
      Their BLOCK_ERROR handling does now follow a common pattern.
      Erroneous blocks are no longer put to the LRU ring, but freed.
      
      2. After fixing the above, accesses to a restored compressed table
      failed with error messages about a corrupted table. Restore of a
      table works by dropping the old table if exists, creating a new
      one, and loading the data from the backup image. The MyISAM
      native driver writes directly to the table files, what was there
      at backup time. This works even for compressed tables, as long as
      the table is freshly opened after loading the data. But RESTORE
      needs to keep the table write locked until it is complete. This
      includes keeping the table open. When restore is done, it unlocks
      the tables and requests close. But our table cache does not
      perform a full close. It marks it unused and keeps it open in the
      cache. Later statements reuse the open table. But when the MyISAM
      table format changes, we must force a full close. Since this is
      difficult to detect, we do now always force close of restored
      MyISAM tables, if restored through the native driver.
added:
  mysql-test/r/myisam_keycache_coverage.result
  mysql-test/suite/backup/r/backup_myisam3.result
  mysql-test/suite/backup/r/backup_myisam3_coverage.result
  mysql-test/suite/backup/t/backup_myisam3.test
  mysql-test/suite/backup/t/backup_myisam3_coverage.test
  mysql-test/t/myisam_keycache_coverage.test
modified:
  .bzrignore
  mysql-test/lib/mtr_report.pl
  mysql-test/suite/backup/r/backup_myisam1.result
  mysql-test/suite/backup/t/backup_myisam1.test
  mysys/mf_keycache.c
  sql/backup/kernel.cc
  storage/myisam/mi_close.c
  storage/myisam/mi_open.c
  storage/myisam/myisam_backup_engine.cc

per-file messages:
  .bzrignore
    Bug#40944 - Backup: crash after myisampack
    Added maria_recovery.trace
    Added unittest/tmp
  mysql-test/lib/mtr_report.pl
    Bug#40944 - Backup: crash after myisampack
    Added a filter for intentionally provoked errors
  mysql-test/r/myisam_keycache_coverage.result
    Bug#40944 - Backup: crash after myisampack
    New test result.
  mysql-test/suite/backup/r/backup_myisam1.result
    Bug#40944 - Backup: crash after myisampack
    Updated test result.
  mysql-test/suite/backup/r/backup_myisam3.result
    Bug#40944 - Backup: crash after myisampack
    New test result.
  mysql-test/suite/backup/r/backup_myisam3_coverage.result
    Bug#40944 - Backup: crash after myisampack
    New test result.
  mysql-test/suite/backup/t/backup_myisam1.test
    Bug#40944 - Backup: crash after myisampack
    Changed case.
    Added USE test to cleanup section.
  mysql-test/suite/backup/t/backup_myisam3.test
    Bug#40944 - Backup: crash after myisampack
    New test case.
  mysql-test/suite/backup/t/backup_myisam3_coverage.test
    Bug#40944 - Backup: crash after myisampack
    New test case.
  mysql-test/t/myisam_keycache_coverage.test
    Bug#40944 - Backup: crash after myisampack
    New test case.
  mysys/mf_keycache.c
    Bug#40944 - Backup: crash after myisampack
    Fixed error handling in unreg_request(), wait_for_readers(),
    key_cache_read(), key_cache_insert(), key_cache_write(), and
    free_block().
  sql/backup/kernel.cc
    Bug#40944 - Backup: crash after myisampack
    Added forced refresh for all tables restored with the
    MyISAM native driver.
  storage/myisam/mi_close.c
    Bug#40944 - Backup: crash after myisampack
    Added DBUG.
  storage/myisam/mi_open.c
    Bug#40944 - Backup: crash after myisampack
    Added DBUG.
  storage/myisam/myisam_backup_engine.cc
    Bug#40944 - Backup: crash after myisampack
    Added a comment.
=== modified file '.bzrignore'
--- a/.bzrignore	2008-10-29 14:20:12 +0000
+++ b/.bzrignore	2008-12-03 16:02:14 +0000
@@ -764,6 +764,7 @@ mysql-test/install_test_db
 mysql-test/lib/init_db.sql
 mysql-test/linux_sys_vars.inc
 mysql-test/load_sysvars.inc
+mysql-test/maria_recovery.trace
 mysql-test/mtr
 mysql-test/mysql-test-run
 mysql-test/mysql-test-run-shell
@@ -1903,3 +1904,4 @@ libmysqld/transaction.cc
 libmysqld/rpl_handler.cc
 libmysql/probes.h
 libmysql_r/probes.h
+unittest/tmp

=== modified file 'mysql-test/lib/mtr_report.pl'
--- a/mysql-test/lib/mtr_report.pl	2008-11-21 15:02:34 +0000
+++ b/mysql-test/lib/mtr_report.pl	2008-12-03 16:02:14 +0000
@@ -365,6 +365,14 @@ sub mtr_report_stats ($) {
 		  /Restore: Open and lock tables failed in RESTORE/
 		) or
                 
+		# myisam_keycache_coverage intentionally provokes errors
+		($testname eq 'main.myisam_keycache_coverage') and
+		(
+		  /Incorrect key file/ or
+		  /Got an error from .* mi_update/ or
+		  /MySQL thread id .* localhost root Updating/
+		) or
+                
 		# The tablespace test triggers error below on purpose
 		($testname eq 'backup.backup_tablespace') and
 		(

=== added file 'mysql-test/r/myisam_keycache_coverage.result'
--- a/mysql-test/r/myisam_keycache_coverage.result	1970-01-01 00:00:00 +0000
+++ b/mysql-test/r/myisam_keycache_coverage.result	2008-12-03 16:02:14 +0000
@@ -0,0 +1,52 @@
+#
+# MyISAM keycache coverage tests.
+#
+DROP TABLE IF EXISTS t1;
+CREATE TABLE t1 (c1 VARCHAR(5), c2 int);
+CREATE INDEX i1 ON t1 (c1, c2);
+INSERT INTO t1 VALUES ('A',1);
+INSERT INTO t1 SELECT * FROM t1;
+INSERT INTO t1 SELECT * FROM t1;
+INSERT INTO t1 SELECT * FROM t1;
+#
+# Positive tests.
+#
+SELECT COUNT(*) FROM t1 WHERE c1 < 5;
+COUNT(*)
+8
+LOAD INDEX INTO CACHE t1;
+Table	Op	Msg_type	Msg_text
+test.t1	preload_keys	status	OK
+UPDATE t1 SET c2=2;
+#
+# Close table and clear cache.
+#
+FLUSH TABLE t1;
+#
+# Inject error key_cache_read_block_error
+#
+SET debug='d,key_cache_read_block_error';
+SELECT COUNT(*) FROM t1 WHERE c1 < 5;
+ERROR HY000: Incorrect key file for table 't1.MYI'; try to repair it
+FLUSH TABLE t1;
+#
+# Inject error key_cache_insert_block_error
+#
+SET debug='d,key_cache_insert_block_error';
+LOAD INDEX INTO CACHE t1;
+Table	Op	Msg_type	Msg_text
+test.t1	preload_keys	error	Failed to read from index file (errno: 5)
+test.t1	preload_keys	status	Operation failed
+FLUSH TABLE t1;
+#
+# Inject error key_cache_write_block_error
+#
+SET debug='d,key_cache_write_block_error';
+UPDATE t1 SET c2=1;
+ERROR HY000: Incorrect key file for table 't1.MYI'; try to repair it
+FLUSH TABLE t1;
+#
+# Cleanup
+#
+SET debug='';
+DROP TABLE t1;

=== modified file 'mysql-test/suite/backup/r/backup_myisam1.result'
--- a/mysql-test/suite/backup/r/backup_myisam1.result	2008-10-07 17:15:44 +0000
+++ b/mysql-test/suite/backup/r/backup_myisam1.result	2008-12-03 16:02:14 +0000
@@ -1,7 +1,8 @@
-drop database if exists mysqltest;
-create database mysqltest;
-use mysqltest;
+DROP DATABASE IF EXISTS mysqltest;
+CREATE DATABASE mysqltest;
+USE mysqltest;
 CREATE TABLE t1 (a int) engine=myisam;
 BACKUP DATABASE mysqltest TO 'test.ba';
 ERROR HY000: Got error -1 'online backup impossible with --external-locking' from MyISAM
+USE test;
 DROP DATABASE mysqltest;

=== added file 'mysql-test/suite/backup/r/backup_myisam3.result'
--- a/mysql-test/suite/backup/r/backup_myisam3.result	1970-01-01 00:00:00 +0000
+++ b/mysql-test/suite/backup/r/backup_myisam3.result	2008-12-03 16:02:14 +0000
@@ -0,0 +1,24 @@
+DROP DATABASE IF EXISTS mysql_db1;
+CREATE DATABASE mysql_db1;
+CREATE TABLE mysql_db1.t1 (c1 VARCHAR(5), c2 int);
+CREATE INDEX i1 ON mysql_db1.t1 (c1, c2);
+INSERT INTO mysql_db1.t1 VALUES ('A',1);
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+FLUSH TABLE mysql_db1.t1;
+BACKUP DATABASE mysql_db1 to 'bup_myisam3.bak';
+backup_id
+#
+RESTORE FROM 'bup_myisam3.bak' OVERWRITE;
+backup_id
+#
+SELECT COUNT(*) FROM mysql_db1.t1 WHERE c1 < 5;
+COUNT(*)
+128
+DROP TABLE mysql_db1.t1;
+DROP DATABASE mysql_db1;

=== added file 'mysql-test/suite/backup/r/backup_myisam3_coverage.result'
--- a/mysql-test/suite/backup/r/backup_myisam3_coverage.result	1970-01-01 00:00:00 +0000
+++ b/mysql-test/suite/backup/r/backup_myisam3_coverage.result	2008-12-03 16:02:14 +0000
@@ -0,0 +1,26 @@
+DROP DATABASE IF EXISTS mysql_db1;
+CREATE DATABASE mysql_db1;
+CREATE TABLE mysql_db1.t1 (c1 VARCHAR(5), c2 int);
+CREATE INDEX i1 ON mysql_db1.t1 (c1, c2);
+INSERT INTO mysql_db1.t1 VALUES ('A',1);
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+FLUSH TABLE mysql_db1.t1;
+BACKUP DATABASE mysql_db1 to 'bup_myisam3.bak';
+backup_id
+#
+SET debug='d,Backup_restore_ctx_unlock_tables_alloc';
+RESTORE FROM 'bup_myisam3.bak' OVERWRITE;
+backup_id
+#
+SELECT COUNT(*) FROM mysql_db1.t1 WHERE c1 < 5;
+COUNT(*)
+128
+SET debug='';
+DROP TABLE mysql_db1.t1;
+DROP DATABASE mysql_db1;

=== modified file 'mysql-test/suite/backup/t/backup_myisam1.test'
--- a/mysql-test/suite/backup/t/backup_myisam1.test	2008-11-25 17:44:19 +0000
+++ b/mysql-test/suite/backup/t/backup_myisam1.test	2008-12-03 16:02:14 +0000
@@ -7,13 +7,13 @@
 # Cleanup from former test cases
 #
 --disable_warnings
-drop database if exists mysqltest;
+DROP DATABASE IF EXISTS mysqltest;
 --enable_warnings
 --error 0,1
 --remove_file $MYSQLTEST_VARDIR/master-data/test.ba
 
-create database mysqltest;
-use mysqltest;
+CREATE DATABASE mysqltest;
+USE mysqltest;
 CREATE TABLE t1 (a int) engine=myisam;
 
 --replace_column 1 #
@@ -23,6 +23,7 @@ BACKUP DATABASE mysqltest TO 'test.ba';
 #
 # Cleanup from this test case
 #
+USE test;
 DROP DATABASE mysqltest;
 # Note: The backup file should not exist as BACKUP command failed.
 --error 1

=== added file 'mysql-test/suite/backup/t/backup_myisam3.test'
--- a/mysql-test/suite/backup/t/backup_myisam3.test	1970-01-01 00:00:00 +0000
+++ b/mysql-test/suite/backup/t/backup_myisam3.test	2008-12-03 16:02:14 +0000
@@ -0,0 +1,47 @@
+#
+# Tests with MyISAM native driver.
+#
+
+--source include/not_embedded.inc
+
+#
+# Precautionary cleanup
+#
+--disable_warnings
+DROP DATABASE IF EXISTS mysql_db1;
+--error 0,1
+--remove_file $MYSQLTEST_VARDIR/master-data/bup_myisam3.bak
+--enable_warnings
+
+#
+# Bug#40944 - Backup: crash after myisampack
+#
+CREATE DATABASE mysql_db1;
+CREATE TABLE mysql_db1.t1 (c1 VARCHAR(5), c2 int);
+CREATE INDEX i1 ON mysql_db1.t1 (c1, c2);
+INSERT INTO mysql_db1.t1 VALUES ('A',1);
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+FLUSH TABLE mysql_db1.t1;
+#
+--exec $MYISAMPACK -s $MYSQLTEST_VARDIR/master-data/mysql_db1/t1
+--exec $MYISAMCHK -srq $MYSQLTEST_VARDIR/master-data/mysql_db1/t1
+#
+--replace_column 1 #
+BACKUP DATABASE mysql_db1 to 'bup_myisam3.bak';
+--replace_column 1 #
+RESTORE FROM 'bup_myisam3.bak' OVERWRITE;
+#
+SELECT COUNT(*) FROM mysql_db1.t1 WHERE c1 < 5;
+#
+# Cleanup
+#
+DROP TABLE mysql_db1.t1;
+DROP DATABASE mysql_db1;
+--remove_file $MYSQLTEST_VARDIR/master-data/bup_myisam3.bak
+

=== added file 'mysql-test/suite/backup/t/backup_myisam3_coverage.test'
--- a/mysql-test/suite/backup/t/backup_myisam3_coverage.test	1970-01-01 00:00:00 +0000
+++ b/mysql-test/suite/backup/t/backup_myisam3_coverage.test	2008-12-03 16:02:14 +0000
@@ -0,0 +1,52 @@
+#
+# Tests with MyISAM native driver, coverage testing.
+#
+
+--source include/have_debug.inc
+--source include/not_embedded.inc
+
+#
+# Precautionary cleanup
+#
+--disable_warnings
+DROP DATABASE IF EXISTS mysql_db1;
+--error 0,1
+--remove_file $MYSQLTEST_VARDIR/master-data/bup_myisam3.bak
+--enable_warnings
+
+#
+# Bug#40944 - Backup: crash after myisampack
+#
+CREATE DATABASE mysql_db1;
+CREATE TABLE mysql_db1.t1 (c1 VARCHAR(5), c2 int);
+CREATE INDEX i1 ON mysql_db1.t1 (c1, c2);
+INSERT INTO mysql_db1.t1 VALUES ('A',1);
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+INSERT INTO mysql_db1.t1 SELECT * FROM mysql_db1.t1;
+FLUSH TABLE mysql_db1.t1;
+#
+--exec $MYISAMPACK -s $MYSQLTEST_VARDIR/master-data/mysql_db1/t1
+--exec $MYISAMCHK -srq $MYSQLTEST_VARDIR/master-data/mysql_db1/t1
+#
+--replace_column 1 #
+BACKUP DATABASE mysql_db1 to 'bup_myisam3.bak';
+#
+# Inject error at Backup_restore_ctx_unlock_tables_alloc
+SET debug='d,Backup_restore_ctx_unlock_tables_alloc';
+--replace_column 1 #
+RESTORE FROM 'bup_myisam3.bak' OVERWRITE;
+#
+SELECT COUNT(*) FROM mysql_db1.t1 WHERE c1 < 5;
+#
+# Cleanup
+#
+SET debug='';
+DROP TABLE mysql_db1.t1;
+DROP DATABASE mysql_db1;
+--remove_file $MYSQLTEST_VARDIR/master-data/bup_myisam3.bak
+

=== added file 'mysql-test/t/myisam_keycache_coverage.test'
--- a/mysql-test/t/myisam_keycache_coverage.test	1970-01-01 00:00:00 +0000
+++ b/mysql-test/t/myisam_keycache_coverage.test	2008-12-03 16:02:14 +0000
@@ -0,0 +1,60 @@
+--echo #
+--echo # MyISAM keycache coverage tests.
+--echo #
+
+--source include/have_debug.inc
+
+--disable_warnings
+DROP TABLE IF EXISTS t1;
+--enable_warnings
+
+CREATE TABLE t1 (c1 VARCHAR(5), c2 int);
+CREATE INDEX i1 ON t1 (c1, c2);
+INSERT INTO t1 VALUES ('A',1);
+INSERT INTO t1 SELECT * FROM t1;
+INSERT INTO t1 SELECT * FROM t1;
+INSERT INTO t1 SELECT * FROM t1;
+
+--echo #
+--echo # Positive tests.
+--echo #
+SELECT COUNT(*) FROM t1 WHERE c1 < 5;
+LOAD INDEX INTO CACHE t1;
+UPDATE t1 SET c2=2;
+
+--echo #
+--echo # Close table and clear cache.
+--echo #
+FLUSH TABLE t1;
+
+--echo #
+--echo # Inject error key_cache_read_block_error
+--echo #
+SET debug='d,key_cache_read_block_error';
+--replace_regex /'.*\//'/
+--error 126
+SELECT COUNT(*) FROM t1 WHERE c1 < 5;
+FLUSH TABLE t1;
+
+--echo #
+--echo # Inject error key_cache_insert_block_error
+--echo #
+SET debug='d,key_cache_insert_block_error';
+LOAD INDEX INTO CACHE t1;
+FLUSH TABLE t1;
+
+--echo #
+--echo # Inject error key_cache_write_block_error
+--echo #
+SET debug='d,key_cache_write_block_error';
+--replace_regex /'.*\//'/
+--error 126
+UPDATE t1 SET c2=1;
+FLUSH TABLE t1;
+
+--echo #
+--echo # Cleanup
+--echo #
+SET debug='';
+DROP TABLE t1;
+

=== modified file 'mysys/mf_keycache.c'
--- a/mysys/mf_keycache.c	2008-10-20 09:16:47 +0000
+++ b/mysys/mf_keycache.c	2008-12-03 16:02:14 +0000
@@ -1373,7 +1373,11 @@ static void unreg_request(KEY_CACHE *key
   DBUG_ASSERT(block->prev_changed && *block->prev_changed == block);
   DBUG_ASSERT(!block->next_used);
   DBUG_ASSERT(!block->prev_used);
-  if (! --block->requests)
+  /*
+    Unregister the request, but do not link erroneous blocks into the
+    LRU ring.
+  */
+  if (!--block->requests && !(block->status & BLOCK_ERROR))
   {
     my_bool hot;
     if (block->hits_left)
@@ -1455,8 +1459,7 @@ static void wait_for_readers(KEY_CACHE *
 #ifdef THREAD
   struct st_my_thread_var *thread= my_thread_var;
   DBUG_ASSERT(block->status & (BLOCK_READ | BLOCK_IN_USE));
-  DBUG_ASSERT(!(block->status & (BLOCK_ERROR | BLOCK_IN_FLUSH |
-                                 BLOCK_CHANGED)));
+  DBUG_ASSERT(!(block->status & (BLOCK_IN_FLUSH | BLOCK_CHANGED)));
   DBUG_ASSERT(block->hash_link);
   DBUG_ASSERT(block->hash_link->block == block);
   /* Linked in file_blocks or changed_blocks hash. */
@@ -2567,7 +2570,6 @@ uchar *key_cache_read(KEY_CACHE *keycach
     reg1 BLOCK_LINK *block;
     uint read_length;
     uint offset;
-    uint status;
     int page_st;
 
     /*
@@ -2665,7 +2667,7 @@ uchar *key_cache_read(KEY_CACHE *keycach
       }
 
       /* block status may have added BLOCK_ERROR in the above 'if'. */
-      if (!((status= block->status) & BLOCK_ERROR))
+      if (!(block->status & BLOCK_ERROR))
       {
 #ifndef THREAD
         if (! return_buffer)
@@ -2691,14 +2693,22 @@ uchar *key_cache_read(KEY_CACHE *keycach
 
       remove_reader(block);
 
-      /*
-         Link the block into the LRU ring if it's the last submitted
-         request for the block. This enables eviction for the block.
-           */
-      unreg_request(keycache, block, 1);
+      /* Error injection for coverage testing. */
+      DBUG_EXECUTE_IF("key_cache_read_block_error",
+                      block->status|= BLOCK_ERROR;);
 
-      if (status & BLOCK_ERROR)
+      /* Do not link erroneous blocks into the LRU ring, but free them. */
+      if (!(block->status & BLOCK_ERROR))
+      {
+        /*
+          Link the block into the LRU ring if it's the last submitted
+          request for the block. This enables eviction for the block.
+        */
+        unreg_request(keycache, block, 1);
+      }
+      else
       {
+        free_block(keycache, block);
         error= 1;
         break;
       }
@@ -2947,16 +2957,25 @@ int key_cache_insert(KEY_CACHE *keycache
 
       remove_reader(block);
 
-      /*
-         Link the block into the LRU ring if it's the last submitted
-         request for the block. This enables eviction for the block.
-      */
-      unreg_request(keycache, block, 1);
-
-      error= (block->status & BLOCK_ERROR);
+      /* Error injection for coverage testing. */
+      DBUG_EXECUTE_IF("key_cache_insert_block_error",
+                      block->status|= BLOCK_ERROR; errno=EIO;);
 
-      if (error)
+      /* Do not link erroneous blocks into the LRU ring, but free them. */
+      if (!(block->status & BLOCK_ERROR))
+      {
+        /*
+          Link the block into the LRU ring if it's the last submitted
+          request for the block. This enables eviction for the block.
+        */
+        unreg_request(keycache, block, 1);
+      }
+      else
+      {
+        free_block(keycache, block);
+        error= 1;
         break;
+      }
 
       buff+= read_length;
       filepos+= read_length+offset;
@@ -3245,14 +3264,24 @@ int key_cache_write(KEY_CACHE *keycache,
       */
       remove_reader(block);
 
-      /*
-         Link the block into the LRU ring if it's the last submitted
-         request for the block. This enables eviction for the block.
-      */
-      unreg_request(keycache, block, 1);
+      /* Error injection for coverage testing. */
+      DBUG_EXECUTE_IF("key_cache_write_block_error",
+                      block->status|= BLOCK_ERROR;);
 
-      if (block->status & BLOCK_ERROR)
+      /* Do not link erroneous blocks into the LRU ring, but free them. */
+      if (!(block->status & BLOCK_ERROR))
       {
+        /*
+          Link the block into the LRU ring if it's the last submitted
+          request for the block. This enables eviction for the block.
+        */
+        unreg_request(keycache, block, 1);
+      }
+      else
+      {
+        /* Pretend a "clean" block to avoid complications. */
+        block->status&= ~(BLOCK_CHANGED);
+        free_block(keycache, block);
         error= 1;
         break;
       }
@@ -3328,8 +3357,9 @@ static void free_block(KEY_CACHE *keycac
 {
   KEYCACHE_THREAD_TRACE("free block");
   KEYCACHE_DBUG_PRINT("free_block",
-                      ("block %u to be freed, hash_link %p",
-                       BLOCK_NUMBER(block), block->hash_link));
+                      ("block %u to be freed, hash_link %p  status: %u",
+                       BLOCK_NUMBER(block), block->hash_link,
+                       block->status));
   /*
     Assert that the block is not free already. And that it is in a clean
     state. Note that the block might just be assigned to a hash_link and
@@ -3411,10 +3441,14 @@ static void free_block(KEY_CACHE *keycac
   if (block->status & BLOCK_IN_EVICTION)
     return;
 
-  /* Here the block must be in the LRU ring. Unlink it again. */
-  DBUG_ASSERT(block->next_used && block->prev_used &&
-              *block->prev_used == block);
-  unlink_block(keycache, block);
+  /* Error blocks are not put into the LRU ring. */
+  if (!(block->status & BLOCK_ERROR))
+  {
+    /* Here the block must be in the LRU ring. Unlink it again. */
+    DBUG_ASSERT(block->next_used && block->prev_used &&
+                *block->prev_used == block);
+    unlink_block(keycache, block);
+  }
   if (block->temperature == BLOCK_WARM)
     keycache->warm_blocks--;
   block->temperature= BLOCK_COLD;

=== modified file 'sql/backup/kernel.cc'
--- a/sql/backup/kernel.cc	2008-11-28 10:10:39 +0000
+++ b/sql/backup/kernel.cc	2008-12-03 16:02:14 +0000
@@ -893,12 +893,68 @@ int Backup_restore_ctx::lock_tables_for_
  */ 
 void Backup_restore_ctx::unlock_tables()
 {
+  TABLE_LIST *tables= NULL;
+
   // Do nothing if tables are not locked.
   if (!m_tables_locked)
     return;
 
   DBUG_PRINT("restore",("unlocking tables"));
 
+  /*
+    Refresh tables that have been restored with the MyISAM native
+    driver. MyISAM native driver might restore table layout that differs
+    from the version created by materialize(). We need to force a final
+    close after restore with close_cached_tables(). Note that we do this
+    before we unlock the tables. Otherwise other threads could use the
+    still open tables before we refresh them.
+
+    For more information about this problem see the comment in
+    myisam_backup_engine.cc:Table_restore::close().
+
+    Collect the list of affected tables.
+  */
+  for (uint snum= 0; snum < m_catalog->snap_count(); ++snum)
+  {
+    backup::Snapshot_info *snap= m_catalog->m_snap[snum];
+
+    if (snap->type() == backup::Snapshot_info::NATIVE_SNAPSHOT)
+    {
+      for (ulong tnum=0; tnum < snap->table_count(); ++tnum)
+      {
+        backup::Image_info::Table *btbl= snap->get_table(tnum);
+        DBUG_ASSERT(btbl); // All tables should be present in the catalogue.
+
+        if (btbl->m_table->table &&
+            (btbl->m_table->table->s->db_type() == myisam_hton))
+        {
+          TABLE_LIST *tlist= (TABLE_LIST*) alloc_root(m_thd->mem_root,
+                                                    sizeof(TABLE_LIST));
+          DBUG_EXECUTE_IF("Backup_restore_ctx_unlock_tables_alloc",
+                          tlist= NULL;);
+          if (!tlist)
+          {
+            /*
+              Do not use fatal_error() here. We won't set
+              Diagnostics_area then. Just flush all tables to be safe.
+            */
+            close_cached_tables(m_thd, NULL, FALSE, FALSE);
+            goto close;
+          }
+          bzero(tlist, sizeof(TABLE_LIST));
+          tlist->table_name= const_cast<char*>(btbl->name().ptr());
+          tlist->alias= tlist->table_name;
+          tlist->db= const_cast<char*>(btbl->db().name().ptr());
+          tables= backup::link_table_list(*tlist, tables); // Never errors
+        }
+      }
+    }
+  }
+  /* Refresh all affected tables, if any. */
+  if (tables)
+    close_cached_tables(m_thd, tables, FALSE, FALSE);
+
+ close:
   close_thread_tables(m_thd);                   // Never errors
   m_tables_locked= FALSE;
 

=== modified file 'storage/myisam/mi_close.c'
--- a/storage/myisam/mi_close.c	2008-07-09 07:12:43 +0000
+++ b/storage/myisam/mi_close.c	2008-12-03 16:02:14 +0000
@@ -30,6 +30,7 @@ int mi_close(register MI_INFO *info)
   DBUG_PRINT("enter",("base: %p  reopen: %u  locks: %u",
 		      info, (uint) share->reopen,
                       (uint) share->tot_locks));
+  DBUG_PRINT("myisam", ("close '%s'", share->unresolv_file_name));
 
   pthread_mutex_lock(&THR_LOCK_myisam);
   if (info->lock_type == F_EXTRA_LCK)
@@ -65,6 +66,7 @@ int mi_close(register MI_INFO *info)
   my_free(mi_get_rec_buff_ptr(info, info->rec_buff), MYF(MY_ALLOW_ZERO_PTR));
   if (flag)
   {
+    DBUG_PRINT("myisam", ("close share '%s'", share->unresolv_file_name));
     if (share->kfile >= 0 &&
 	flush_key_blocks(share->key_cache, share->kfile,
 			 share->temporary ? FLUSH_IGNORE_CHANGED :

=== modified file 'storage/myisam/mi_open.c'
--- a/storage/myisam/mi_open.c	2008-10-20 09:16:47 +0000
+++ b/storage/myisam/mi_open.c	2008-12-03 16:02:14 +0000
@@ -89,6 +89,7 @@ MI_INFO *mi_open(const char *name, int m
   my_off_t key_root[HA_MAX_POSSIBLE_KEY],key_del[MI_MAX_KEY_BLOCK_SIZE];
   ulonglong max_key_file_length, max_data_file_length;
   DBUG_ENTER("mi_open");
+  DBUG_PRINT("myisam", ("open '%s'", name));
 
   LINT_INIT(m_info);
   kfile= -1;
@@ -115,6 +116,7 @@ MI_INFO *mi_open(const char *name, int m
   pthread_mutex_lock(&THR_LOCK_myisam);
   if (!(old_info=test_if_reopen(name_buff)))
   {
+    DBUG_PRINT("myisam", ("open share '%s'", name));
     share= &share_buff;
     bzero((uchar*) &share_buff,sizeof(share_buff));
     share_buff.state.rec_per_key_part=rec_per_key_part;

=== modified file 'storage/myisam/myisam_backup_engine.cc'
--- a/storage/myisam/myisam_backup_engine.cc	2008-08-11 16:06:30 +0000
+++ b/storage/myisam/myisam_backup_engine.cc	2008-12-03 16:02:14 +0000
@@ -1754,6 +1754,25 @@ result_t Table_restore::close()
     But since the share does now cache the new values from the
     index file, the backup kernel's close writes the correct
     information back to the file.
+
+    This used to work until a brave soul tried to backup and restore
+    compressed tables. Now we know, that replacing the state info is
+    insufficient. The table is always re-created as a non-compressed
+    table. The setup of the share is pretty different between normal and
+    compressed tables. We could try to replace all relevant information.
+    But that would make quite some code duplication with mi_open().
+    Changes there might be forgotten here. And it might still be
+    insufficient. The table instance MI_OPEN might have some setup
+    differences too. Perhaps even the handler ha_myisam. In theory it
+    might even happen that we create fixed length records, while the
+    restored MYI has dynamic records or vice versa. Or we restore a
+    table that had been created by a former MySQL version and has
+    different field types, e.g. varchar.
+
+    So the only practical solution seems to be to re-open the table
+    after restore. But this must be done in the server. The fix here is
+    still required to defeat writing of wrong share data at close as
+    decribed above.
   */
   {
     MI_INFO      *mi_info;

Thread
bzr commit into mysql-6.0-backup branch (ingo.struewing:2737) Bug#40944Ingo Struewing3 Dec
  • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2737)Bug#40944Øystein Grøvlen18 Dec
    • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2737)Bug#40944Ingo Strüwing18 Dec
      • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2737)Bug#40944Øystein Grøvlen19 Dec
  • Bug still exists Re: bzr commit into mysql-6.0-backup branch(ingo.struewing:2737) Bug#40944Guilhem Bichot6 Feb
    • Re: Bug still exists Re: bzr commit into mysql-6.0-backup branch(ingo.struewing:2737) Bug#40944Ingo Strüwing6 Feb
      • Re: Bug still exists Re: bzr commit into mysql-6.0-backup branch(ingo.struewing:2737) Bug#40944Rafal Somla6 Feb