bzr commit into mysql-backup-backport branch (ingo.struewing:3023) Bug#42519 WL#5101

[Ingo Struewing]

#At file:///home2/mydev/bzrroot/mysql-5.6-backup-backport-ms09-2/ based on revid:ingo.struewing@stripped

 3023 Ingo Struewing	2010-01-04
      WL#5101 - MySQL Backup back port - MS09
      Merged (part of) revid:guilhem@stripped
        Maria online backup: BUG#42519 "Maria: RESTORE leads to corrupted table and assertion":
        * working around deficiencies of the restore kernel
        (occuring also in
        BUG 41716 "Backup Error when sending data (for table #1) to Default/Snapshot restore driver"
        BUG 40944 "Backup: crash after myisampack"); this fixes BUG#42519.
        * same workaround for MyISAM (which suffered from a bug similar to BUG#42519).
        * fixing a Maria restore driver bug found during debugging this ("DELETE FROM <table>;" was
        not handled during backup).
      
      Only included the changes in storage/myisam/myisam_backup_engine.cc.
      The changes from sql/backup/kernel.cc.
      Left away all maria changes.
      
      original changeset: 2617.26.9
     @ storage/myisam/myisam_backup_engine.cc
        WL#5101 - MySQL Backup back port - MS09
            Same changes as in maria_backup_engine.cc (though simplified).
        Comments from storage/maria/maria_backup_engine.cc:
            * Working around a deficiency of the restore kernel:
             - it leaves a window between creating the table empty and locking it; in this window,
            another thread can do an update to the table (which conflicts with what the restore
            driver will put into files).
             - it does not always really close tables at end of restore, so restore driver has to
            make sure to invalidate any cached info before table is unlocked.
            - see this file's comments for more details about the two problems above;
            they were the causes of BUG#42519 (various corruptions or assertion failures
            in the first DMLs after RESTORE, depending on concurrency).
            * The same logic is needed independently of rebuilding the index or not.
            * keep MARIA_SHARE::close_lock when iterating over states of table,
            to protect against a concurrent checkpoint (safer).

    modified:
      storage/myisam/myisam_backup_engine.cc
=== modified file 'storage/myisam/myisam_backup_engine.cc'
--- a/storage/myisam/myisam_backup_engine.cc	2010-01-04 14:01:39 +0000
+++ b/storage/myisam/myisam_backup_engine.cc	2010-01-04 15:46:18 +0000
@@ -119,6 +119,23 @@ using backup::Buffer;
 */
 #define MYISAM_BACKUP_VERSION 1
 
+/**
+  Restore kernel opens tables and locks them for the duration of restore
+  (after having created them empty); this means that cached objects stay
+  around (MYISAM_SHARE, MI_INFO) and can become out-of-sync with the
+  data/index file filled by the driver, unless we take precautions which are
+  recognizable by this symbol.
+*/
+#define RESTORE_KERNEL_KEEPS_OPEN_TABLES 1
+/**
+  Restore kernel leaves a time windows between end of creation of table (via
+  execution of CREATE TABLE) and locking of this table; in this window another
+  client can open/lock/modify/unlock the table, which conflicts with what the
+  driver is going to write to the data/index file, unless we take precautions
+  which are recognizable by this symbol.
+*/
+#define RESTORE_KERNEL_NOT_ATOMIC 1
+
 /** Like Table_ref but with file name added */
 class Myisam_table_ref
 {
@@ -1664,6 +1681,19 @@ Table_restore::Table_restore(const Table
     /* table does not exist or is corrupted? not normal, it's just created */
     goto err;
   }
+#ifdef RESTORE_KERNEL_NOT_ATOMIC
+  /*
+    Restore kernel leaves a window between creation of table and locking it;
+    in this window, another thread can modify the table, put pages in page
+    cache, alter state, increase the files's length to greater than what the
+    driver has to write...
+    So we re-empty it here. We know we are alone using the table at this
+    point, as restore kernel has finished locking tables.
+    See BUG#42519, BUG#41716.
+  */
+  if (mi_delete_all_rows(mi_info))
+    goto err;
+#endif
   /*
     It's ok to copy the kfile descriptor and write() to it as the upper layers
     guarantee that we are the only user of the brand new table (nobody will
@@ -1717,6 +1747,7 @@ result_t Table_restore::close()
       (kfile_restore.close_file() != backup::OK))
     SET_STATE_TO_ERROR_AND_DBUG_RETURN;
 
+#ifdef RESTORE_KERNEL_KEEPS_OPEN_TABLES
   /*
     CAUTION! Ugliest hack ever!
     This hack tries to recover from bypassing the MyISAM interface
@@ -1809,6 +1840,7 @@ result_t Table_restore::close()
   end :
     do {} while (0); /* Empty statement, syntactically required. */
   }
+#endif
 
   DBUG_RETURN(backup::OK);
 }
@@ -1850,7 +1882,6 @@ result_t Table_restore::post_restore()
   Vio* save_vio;
   DBUG_ENTER("myisam_backup::Table_restore::post_restore");
 
-  if (!rebuild_index)
   {
     MI_INFO *mi_info;
     MYISAM_SHARE *share;
@@ -1877,9 +1908,11 @@ result_t Table_restore::post_restore()
       error= mi_state_info_write(share, share->kfile, &share->state, 1);
     }
     error|= mi_close(mi_info);
-    goto err;
   }
 
+  if (!rebuild_index)
+    goto err;
+
   /*
     myisamchk() as well as ha_myisam::repair() do a lot of operations before
     and after mi_repair(); to not duplicate code we reuse one of them.


Attachment: [text/bzr-bundle] bzr/[e-mail]