From 45f7a939149e457403935db8c6b131baf6193f36 Mon Sep 17 00:00:00 2001 From: Guilhem Bichot Date: Mon, 26 Jan 2009 22:14:43 +0100 Subject: [PATCH 1/4] Putting back some fixes lost in a colleague's merge a while back; that was about problems when the R-tree index is not the first index. --- storage/maria/ma_rt_index.c | 2 +- storage/myisam/rt_index.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/storage/maria/ma_rt_index.c b/storage/maria/ma_rt_index.c index ccdddf5906d..15202c335b2 100644 --- a/storage/maria/ma_rt_index.c +++ b/storage/maria/ma_rt_index.c @@ -433,7 +433,7 @@ int maria_rtree_get_first(MARIA_HA *info, uint keynr, uint key_length) info->maria_rtree_recursion_depth= -1; info->keyread_buff_used= 1; - return maria_rtree_get_req(info, &keyinfo[keynr], key_length, root, 0); + return maria_rtree_get_req(info, keyinfo, key_length, root, 0); } diff --git a/storage/myisam/rt_index.c b/storage/myisam/rt_index.c index b1375d99bd0..822983b718c 100644 --- a/storage/myisam/rt_index.c +++ b/storage/myisam/rt_index.c @@ -378,6 +378,7 @@ err1: int rtree_get_first(MI_INFO *info, uint keynr, uint key_length) { my_off_t root; + MI_KEYDEF *keyinfo = info->s->keyinfo + keynr; if ((root = info->s->state.key_root[keynr]) == HA_OFFSET_ERROR) { @@ -388,7 +389,7 @@ int rtree_get_first(MI_INFO *info, uint keynr, uint key_length) info->rtree_recursion_depth = -1; info->buff_used = 1; - return rtree_get_req(info, info->s->keyinfo, key_length, root, 0); + return rtree_get_req(info, keyinfo, key_length, root, 0); } From 1cd1ae6d4bfb5dcbb84471d421eeb846437a6a8c Mon Sep 17 00:00:00 2001 From: Guilhem Bichot Date: Mon, 26 Jan 2009 22:19:13 +0100 Subject: [PATCH 2/4] In ctype_utf8.test, use a column large enough to provoke a failure in Maria too, not only in MyISAM (this is to ease ./mtr --mysqld=--default-storage-engine=maria) --- mysql-test/r/ctype_utf8.result | 2 +- mysql-test/t/ctype_utf8.test | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mysql-test/r/ctype_utf8.result b/mysql-test/r/ctype_utf8.result index 6f4ae965ca0..4cfd06149f1 100644 --- a/mysql-test/r/ctype_utf8.result +++ b/mysql-test/r/ctype_utf8.result @@ -240,7 +240,7 @@ select hex(s1) from t1; hex(s1) 41 drop table t1; -create table t1 (a text character set utf8, primary key(a(360))); +create table t1 (a text character set utf8, primary key(a(371))); ERROR 42000: Specified key was too long; max key length is 1000 bytes CREATE TABLE t1 ( a varchar(10) ) CHARACTER SET utf8; INSERT INTO t1 VALUES ( 'test' ); diff --git a/mysql-test/t/ctype_utf8.test b/mysql-test/t/ctype_utf8.test index f0c769251cf..bb64319f1e4 100644 --- a/mysql-test/t/ctype_utf8.test +++ b/mysql-test/t/ctype_utf8.test @@ -164,7 +164,7 @@ drop table t1; # UTF8 breaks primary keys for cols > 333 characters # --error 1071 -create table t1 (a text character set utf8, primary key(a(360))); +create table t1 (a text character set utf8, primary key(a(371))); # From 554eb6c2cf00d80d283374a749ca82f1a1f13bae Mon Sep 17 00:00:00 2001 From: Guilhem Bichot Date: Mon, 9 Feb 2009 22:02:04 +0100 Subject: [PATCH 3/4] Comments. Take bitmap mutex lock when changing bitmap.changed. storage/maria/ha_maria.cc: comment storage/maria/ma_checkpoint.c: comment storage/maria/ma_extra.c: use bitmap mutex when changing bitmap.changed, sounds safer storage/maria/ma_pagecache.c: comment storage/maria/ma_recovery.c: comments --- storage/maria/ha_maria.cc | 3 ++- storage/maria/ma_checkpoint.c | 8 ++++++++ storage/maria/ma_extra.c | 6 +++++- storage/maria/ma_pagecache.c | 8 ++++---- storage/maria/ma_recovery.c | 5 ++++- 5 files changed, 23 insertions(+), 7 deletions(-) diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc index 34c7bd1455e..efcfca5bd62 100644 --- a/storage/maria/ha_maria.cc +++ b/storage/maria/ha_maria.cc @@ -2381,7 +2381,8 @@ int ha_maria::external_lock(THD *thd, int lock_type) Note that we can come here without having an exclusive lock on the table, for example in this case: external_lock(F_(WR|RD)LCK); thr_lock() which fails due to lock - abortion; external_lock(F_UNLCK). + abortion; external_lock(F_UNLCK). Fortunately, the re-enabling happens + only if we were the thread which disabled logging. */ if (_ma_reenable_logging_for_table(file, TRUE)) DBUG_RETURN(1); diff --git a/storage/maria/ma_checkpoint.c b/storage/maria/ma_checkpoint.c index a78b31edce7..6d1a6332c54 100644 --- a/storage/maria/ma_checkpoint.c +++ b/storage/maria/ma_checkpoint.c @@ -653,6 +653,14 @@ pthread_handler_t ma_checkpoint_background(void *arg) We use FLUSH_KEEP_LAZY: if a file is already in flush, it's smarter to move to the next file than wait for this one to be completely flushed, which may take long. + StaleFilePointersInFlush: notice how below we use "dfile" which + is an OS file descriptor plus some function and MARIA_SHARE + pointers; this data dates from a previous checkpoint; since then, + the table may have been closed (so MARIA_SHARE* became stale), and + the file descriptor reassigned to another table which does not + have the same CRC-read-set callbacks: it is thus important that + flush_pagecache_blocks_with_filter() does not use the pointers, + only the OS file descriptor. */ int res= flush_pagecache_blocks_with_filter(maria_pagecache, diff --git a/storage/maria/ma_extra.c b/storage/maria/ma_extra.c index c311a035e9b..92e0b0d4e15 100644 --- a/storage/maria/ma_extra.c +++ b/storage/maria/ma_extra.c @@ -592,7 +592,11 @@ int _ma_flush_table_files(MARIA_HA *info, uint flush_data_or_index, error= 1; } else - info->s->bitmap.changed= 0; + { + pthread_mutex_lock(&share->bitmap.bitmap_lock); + share->bitmap.changed= 0; + pthread_mutex_unlock(&share->bitmap.bitmap_lock); + } if (flush_pagecache_blocks(share->pagecache, &info->dfile, flush_type_for_data)) error= 1; diff --git a/storage/maria/ma_pagecache.c b/storage/maria/ma_pagecache.c index 0a5cea8a541..3887d6af7bc 100644 --- a/storage/maria/ma_pagecache.c +++ b/storage/maria/ma_pagecache.c @@ -4227,11 +4227,11 @@ static int flush_cached_blocks(PAGECACHE *pagecache, @todo IO If page is contiguous with next page to flush, group flushes in one single my_pwrite(). */ - /* + /** It is important to use block->hash_link->file below and not 'file', as - the first one is right and the second may have different content (and - this matters for callbacks, bitmap pages and data pages have different - ones). + the first one is right and the second may have different out-of-date + content (see StaleFilePointersInFlush in ma_checkpoint.c). + @todo change argument of functions to be File. */ error= pagecache_fwrite(pagecache, &block->hash_link->file, block->buffer, diff --git a/storage/maria/ma_recovery.c b/storage/maria/ma_recovery.c index 8470f509140..08aba4fdcd6 100644 --- a/storage/maria/ma_recovery.c +++ b/storage/maria/ma_recovery.c @@ -3326,7 +3326,10 @@ void _ma_tmp_disable_logging_for_table(MARIA_HA *info, /** Re-enables logging for a table which had it temporarily disabled. - Only the thread which disabled logging is allowed to reenable it. + Only the thread which disabled logging is allowed to reenable it. Indeed, + re-enabling logging affects all open instances, one must have exclusive + access to the table to do that. In practice, the one which disables has + such access. @param info table @param flush_pages if function needs to flush pages first From 9653feb1600c84fc5e51fe102761c93b7f83de0f Mon Sep 17 00:00:00 2001 From: Guilhem Bichot Date: Mon, 9 Feb 2009 22:52:42 +0100 Subject: [PATCH 4/4] Callers of translog_deassign_id_from_share() need intern_lock. Assert that keys don't point to bitmap pages. storage/maria/ma_blockrec.h: assertion storage/maria/ma_locking.c: With concurrent INSERTs, it is possible that two threads enter _ma_mark_file_changed() at the same time, so they should serialize their access to the "changed" state/share members; another reason is that this function may call _ma_update_state_lsns_sub() which may call translog_deassign_id_from_share() (I saw it during testing of online backup) which requires the intern_lock mutex. As INSERTs only change from "not changed" to "changed", we can first check without mutex: if it says "changed", some other thread has set or is setting the variables now, we don't need to do it; if it says "not changed", we serialize and re-check. --- storage/maria/ma_blockrec.h | 1 + storage/maria/ma_locking.c | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/storage/maria/ma_blockrec.h b/storage/maria/ma_blockrec.h index 7262205d10e..cb682eef701 100644 --- a/storage/maria/ma_blockrec.h +++ b/storage/maria/ma_blockrec.h @@ -122,6 +122,7 @@ static inline MARIA_RECORD_POS ma_recordpos(pgcache_page_no_t page, uint dir_entry) { DBUG_ASSERT(dir_entry <= 255); + DBUG_ASSERT(page > 0); /* page 0 is bitmap, not data page */ return (MARIA_RECORD_POS) (((ulonglong) page << 8) | dir_entry); } diff --git a/storage/maria/ma_locking.c b/storage/maria/ma_locking.c index 2a34d1fe9f3..9d57cd1cba3 100644 --- a/storage/maria/ma_locking.c +++ b/storage/maria/ma_locking.c @@ -391,9 +391,15 @@ int _ma_mark_file_changed(MARIA_HA *info) { uchar buff[3]; register MARIA_SHARE *share= info->s; + int error= 1; DBUG_ENTER("_ma_mark_file_changed"); - if (!(share->state.changed & STATE_CHANGED) || ! share->global_changed) +#define _MA_ALREADY_MARKED_FILE_CHANGED \ + ((share->state.changed & STATE_CHANGED) && share->global_changed) + if (_MA_ALREADY_MARKED_FILE_CHANGED) + DBUG_RETURN(0); + pthread_mutex_lock(&share->intern_lock); /* recheck under mutex */ + if (! _MA_ALREADY_MARKED_FILE_CHANGED) { share->state.changed|=(STATE_CHANGED | STATE_NOT_ANALYZED | STATE_NOT_OPTIMIZED_KEYS); @@ -420,7 +426,7 @@ int _ma_mark_file_changed(MARIA_HA *info) sizeof(share->state.header) + MARIA_FILE_OPEN_COUNT_OFFSET, MYF(MY_NABP))) - DBUG_RETURN(1); + goto err; } /* Set uuid of file if not yet set (zerofilled file) */ if (share->base.born_transactional && @@ -432,11 +438,15 @@ int _ma_mark_file_changed(MARIA_HA *info) _ma_update_state_lsns_sub(share, LSN_IMPOSSIBLE, trnman_get_min_trid(), TRUE, TRUE))) - DBUG_RETURN(1); + goto err; share->state.changed|= STATE_NOT_MOVABLE; } } - DBUG_RETURN(0); + error= 0; +err: + pthread_mutex_unlock(&share->intern_lock); + DBUG_RETURN(error); +#undef _MA_ALREADY_MARKED_FILE_CHANGED } /*