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.
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
Question for reviewer: if we enter the if() branch, do we also need to do:
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,
+ can_enable_indexes= (maria_is_all_keys_active(share->state.key_map,
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.
- if (file->s->now_transactional)
+ if (share->now_transactional)
|• bzr commit into MySQL/Maria:mysql-maria branch (guilhem:2680) Bug#39710||Guilhem Bichot||16 Oct|