List:Maria Storage Engine« Previous MessageNext Message »
From:Guilhem Bichot Date:October 16 2008 7:48pm
Subject:bzr commit into MySQL/Maria:mysql-maria branch (guilhem:2680) Bug#39710
View as plain text  
#At bzr+ssh://bk-internal.mysql.com/bzrroot/server/mysql-maria/

 2680 Guilhem Bichot	2008-10-16
      Fix for BUG#39710 "Maria assertion in maria_disable_non_unique_index".
      No testcase, this requires concurrency and is automatically tested by
      maria_bulk_insert.yy in pushbuild2.
modified:
  storage/maria/ha_maria.cc

per-file messages:
  storage/maria/ha_maria.cc
    The case of BUG#39710 is:
    two threads want to INSERT SELECT into the same table.
    Thread1 (T1) starts, does external_lock, thr_lock (store_lock sees 0 records so
    upgrades to TL_WRITE), goes into bulk insert, starts writes
    T2 starts, external_lock, thr_lock (store_lock sees 0 records so
    upgrades to TL_WRITE), blocks on existing thr_lock of T1.
    T1 ends writes, ends bulk insert, commits (ha_maria::implicit_commit()
    at end of dispatch_command()), external_lock and thr_unlock
    (close_thread_tables() at end of dispatch_command()).
    T2 wakes up, gets thr_lock, goes into start_bulk_insert() where
    file->state is out-of-date and still says that file->state->records==0,
    so maria_disable_non_unique_index() is called, which asserts because
    the actual number of records (share->state.state.records) is >0.
    The solution, maybe temporary, is to also check share->state.state.records==0
    when deciding to do bulk insert, with the idea that such operation cannot
    rely on the view of the start of the transaction, as it uses repair,
    and can safely read share->state as it has acquired the exclusive
    TL_WRITE.
    Question for reviewer: if we enter the if() branch, do we also need to do:
    *(file->state)= share->state.state;
    or even call some existing function which does that?
=== modified file 'storage/maria/ha_maria.cc'
--- a/storage/maria/ha_maria.cc	2008-10-14 21:23:33 +0000
+++ b/storage/maria/ha_maria.cc	2008-10-16 19:48:32 +0000
@@ -1795,6 +1795,7 @@ void ha_maria::start_bulk_insert(ha_rows
   THD *thd= current_thd;
   ulong size= min(thd->variables.read_buff_size,
                   (ulong) (table->s->avg_row_length * rows));
+  MARIA_SHARE *share= file->s;
   DBUG_PRINT("info", ("start_bulk_insert: rows %lu size %lu",
                       (ulong) rows, size));
 
@@ -1802,8 +1803,8 @@ void ha_maria::start_bulk_insert(ha_rows
   if (!rows || (rows > MARIA_MIN_ROWS_TO_USE_WRITE_CACHE))
     maria_extra(file, HA_EXTRA_WRITE_CACHE, (void*) &size);
 
-  can_enable_indexes= (maria_is_all_keys_active(file->s->state.key_map,
-                                                file->s->base.keys));
+  can_enable_indexes= (maria_is_all_keys_active(share->state.key_map,
+                                                share->base.keys));
   bulk_insert_single_undo= BULK_INSERT_NONE;
 
   if (!(specialflag & SPECIAL_SAFE_MODE))
@@ -1815,8 +1816,17 @@ void ha_maria::start_bulk_insert(ha_rows
        we don't want to update the key statistics based of only a few rows.
        Index file rebuild requires an exclusive lock, so if versioning is on
        don't do it (see how ha_maria::store_lock() tries to predict repair).
+       We can repair index only if we have an exclusive (TL_WRITE) lock. To
+       see if table is empty, we shouldn't rely on the old records' count from
+       our transaction's start (if that old count is 0 but now there are
+       records in the table, we would wrongly destroy them).
+       So we need to look at share->state.state.records.
+       As a safety net for now, we don't remove the test of
+       file->state->records, because there is uncertainty on what will happen
+       during repair if the two states disagree.
     */
-    if (file->state->records == 0 && can_enable_indexes &&
+    if ((file->state->records == 0) &&
+        (share->state.state.records == 0) && can_enable_indexes &&
         (!rows || rows >= MARIA_MIN_ROWS_TO_DISABLE_INDEXES) &&
         (file->lock.type == TL_WRITE))
     {
@@ -1825,7 +1835,7 @@ void ha_maria::start_bulk_insert(ha_rows
          is more costly (flushes, syncs) than a row write.
       */
       maria_disable_non_unique_index(file, rows);
-      if (file->s->now_transactional)
+      if (share->now_transactional)
       {
         bulk_insert_single_undo= BULK_INSERT_SINGLE_UNDO_AND_NO_REPAIR;
         write_log_record_for_bulk_insert(file);

Thread
bzr commit into MySQL/Maria:mysql-maria branch (guilhem:2680) Bug#39710Guilhem Bichot16 Oct