Commit d95cbe36 authored by Michael Widenius's avatar Michael Widenius

Fixed bug discovered by testcase for LP#618558 (original bug seams to be fixed):

- Fixed bug in pagecache_delete_internal() when deleting block that was flushed by another thread (fixed bug when block->next_used was unexpectedly null)
Fixed some using uninitialized memory warnings found by valgrind. 

mysql-test/t/information_schema_all_engines-master.opt:
  Added options to make slow test run faster
sql/sp.cc:
  Fixed valgrind warning.
sql/sql_show.cc:
  Fixed valgrind warning.
storage/maria/ma_bitmap.c:
  Fixed wrong call parameter to pagecache_unlock_by_link() that caused assert crash in pagecache
storage/maria/ma_pagecache.c:
  Fixed possible error in pagecache_wait_lock() when hash_link was not set. (We never hit this issue, but could be possible when two threads updates the same page.
  Fixed bug in pagecache_delete_internal() when deleting block that was flushed by another thread (fixed bug when block->next_used was unexpectedly null)
  Cleanup: moved pagecache_pthread_mutex_unlock() over comments and asserts to be just before pagecache_fwrite()
parent 9d68ccde
--skip-safemalloc --mutex-deadlock-detector=0
...@@ -783,7 +783,7 @@ db_load_routine(THD *thd, int type, sp_name *name, sp_head **sphp, ...@@ -783,7 +783,7 @@ db_load_routine(THD *thd, int type, sp_name *name, sp_head **sphp,
{ {
Parser_state parser_state; Parser_state parser_state;
if (parser_state.init(thd, defstr.c_ptr(), defstr.length())) if (parser_state.init(thd, defstr.c_ptr_safe(), defstr.length()))
{ {
ret= SP_INTERNAL_ERROR; ret= SP_INTERNAL_ERROR;
goto end; goto end;
......
...@@ -3944,7 +3944,7 @@ static int get_schema_column_record(THD *thd, TABLE_LIST *tables, ...@@ -3944,7 +3944,7 @@ static int get_schema_column_record(THD *thd, TABLE_LIST *tables,
base_type [(dimension)] [unsigned] [zerofill]. base_type [(dimension)] [unsigned] [zerofill].
For DATA_TYPE column we extract only base type. For DATA_TYPE column we extract only base type.
*/ */
tmp_buff= strchr(type.ptr(), '('); tmp_buff= strchr(type.c_ptr_safe(), '(');
if (!tmp_buff) if (!tmp_buff)
/* /*
if there is no dimention part then check the presence of if there is no dimention part then check the presence of
......
...@@ -492,7 +492,7 @@ static void _ma_bitmap_unpin_all(MARIA_SHARE *share) ...@@ -492,7 +492,7 @@ static void _ma_bitmap_unpin_all(MARIA_SHARE *share)
while (pinned_page-- != page_link) while (pinned_page-- != page_link)
pagecache_unlock_by_link(share->pagecache, pinned_page->link, pagecache_unlock_by_link(share->pagecache, pinned_page->link,
pinned_page->unlock, PAGECACHE_UNPIN, pinned_page->unlock, PAGECACHE_UNPIN,
LSN_IMPOSSIBLE, LSN_IMPOSSIBLE, TRUE, TRUE); LSN_IMPOSSIBLE, LSN_IMPOSSIBLE, FALSE, TRUE);
bitmap->pinned_pages.elements= 0; bitmap->pinned_pages.elements= 0;
DBUG_VOID_RETURN; DBUG_VOID_RETURN;
} }
......
...@@ -2037,12 +2037,12 @@ restart: ...@@ -2037,12 +2037,12 @@ restart:
KEYCACHE_DBUG_PRINT("find_block", ("block is dirty")); KEYCACHE_DBUG_PRINT("find_block", ("block is dirty"));
pagecache_pthread_mutex_unlock(&pagecache->cache_lock);
/* /*
The call is thread safe because only the current The call is thread safe because only the current
thread might change the block->hash_link value thread might change the block->hash_link value
*/ */
DBUG_ASSERT(block->pins == 0); DBUG_ASSERT(block->pins == 0);
pagecache_pthread_mutex_unlock(&pagecache->cache_lock);
error= pagecache_fwrite(pagecache, error= pagecache_fwrite(pagecache,
&block->hash_link->file, &block->hash_link->file,
block->buffer, block->buffer,
...@@ -2255,14 +2255,17 @@ static my_bool pagecache_wait_lock(PAGECACHE *pagecache, ...@@ -2255,14 +2255,17 @@ static my_bool pagecache_wait_lock(PAGECACHE *pagecache,
#endif #endif
PCBLOCK_INFO(block); PCBLOCK_INFO(block);
if ((block->status & (PCBLOCK_REASSIGNED | PCBLOCK_IN_SWITCH)) || if ((block->status & (PCBLOCK_REASSIGNED | PCBLOCK_IN_SWITCH)) ||
!block->hash_link ||
file.file != block->hash_link->file.file || file.file != block->hash_link->file.file ||
pageno != block->hash_link->pageno) pageno != block->hash_link->pageno)
{ {
DBUG_PRINT("info", ("the block 0x%lx changed => need retry " DBUG_PRINT("info", ("the block 0x%lx changed => need retry "
"status: %x files %d != %d or pages %lu != %lu", "status: %x files %d != %d or pages %lu != %lu",
(ulong)block, block->status, (ulong)block, block->status,
file.file, block->hash_link->file.file, file.file,
(ulong) pageno, (ulong) block->hash_link->pageno)); block->hash_link ? block->hash_link->file.file : -1,
(ulong) pageno,
(ulong) (block->hash_link ? block->hash_link->pageno : 0)));
DBUG_RETURN(1); DBUG_RETURN(1);
} }
DBUG_RETURN(0); DBUG_RETURN(0);
...@@ -2611,12 +2614,12 @@ static void read_block(PAGECACHE *pagecache, ...@@ -2611,12 +2614,12 @@ static void read_block(PAGECACHE *pagecache,
*/ */
pagecache->global_cache_read++; pagecache->global_cache_read++;
/* Page is not in buffer yet, is to be read from disk */
pagecache_pthread_mutex_unlock(&pagecache->cache_lock);
/* /*
Page is not in buffer yet, is to be read from disk
Here other threads may step in and register as secondary readers. Here other threads may step in and register as secondary readers.
They will register in block->wqueue[COND_FOR_REQUESTED]. They will register in block->wqueue[COND_FOR_REQUESTED].
*/ */
pagecache_pthread_mutex_unlock(&pagecache->cache_lock);
error= pagecache_fread(pagecache, &block->hash_link->file, error= pagecache_fread(pagecache, &block->hash_link->file,
block->buffer, block->buffer,
block->hash_link->pageno, block->hash_link->pageno,
...@@ -3450,6 +3453,14 @@ static my_bool pagecache_delete_internal(PAGECACHE *pagecache, ...@@ -3450,6 +3453,14 @@ static my_bool pagecache_delete_internal(PAGECACHE *pagecache,
my_bool flush) my_bool flush)
{ {
my_bool error= 0; my_bool error= 0;
if (block->status & PCBLOCK_IN_FLUSH)
{
/*
this call is just 'hint' for the cache to free the page so we will
not interferes with flushing process but gust return success
*/
goto out;
}
if (block->status & PCBLOCK_CHANGED) if (block->status & PCBLOCK_CHANGED)
{ {
if (flush) if (flush)
...@@ -3458,12 +3469,12 @@ static my_bool pagecache_delete_internal(PAGECACHE *pagecache, ...@@ -3458,12 +3469,12 @@ static my_bool pagecache_delete_internal(PAGECACHE *pagecache,
KEYCACHE_DBUG_PRINT("find_block", ("block is dirty")); KEYCACHE_DBUG_PRINT("find_block", ("block is dirty"));
pagecache_pthread_mutex_unlock(&pagecache->cache_lock);
/* /*
The call is thread safe because only the current The call is thread safe because only the current
thread might change the block->hash_link value thread might change the block->hash_link value
*/ */
DBUG_ASSERT(block->pins == 1); DBUG_ASSERT(block->pins == 1);
pagecache_pthread_mutex_unlock(&pagecache->cache_lock);
error= pagecache_fwrite(pagecache, error= pagecache_fwrite(pagecache,
&block->hash_link->file, &block->hash_link->file,
block->buffer, block->buffer,
...@@ -3478,7 +3489,25 @@ static my_bool pagecache_delete_internal(PAGECACHE *pagecache, ...@@ -3478,7 +3489,25 @@ static my_bool pagecache_delete_internal(PAGECACHE *pagecache,
block->status|= PCBLOCK_ERROR; block->status|= PCBLOCK_ERROR;
block->error= (int16) my_errno; block->error= (int16) my_errno;
my_debug_put_break_here(); my_debug_put_break_here();
goto err; goto out;
}
}
else
{
PAGECACHE_FILE *filedesc= &block->hash_link->file;
/* We are not going to write the page but have to call callbacks */
DBUG_PRINT("info", ("flush_callback :0x%lx data: 0x%lx"
"write_callback: 0x%lx data: 0x%lx",
(ulong) filedesc->write_callback,
(ulong) filedesc->callback_data));
if ((*filedesc->flush_log_callback)
(block->buffer, block->hash_link->pageno, filedesc->callback_data) ||
(*filedesc->write_callback)
(block->buffer, block->hash_link->pageno, filedesc->callback_data))
{
DBUG_PRINT("error", ("flush or write callback problem"));
error= 1;
goto out;
} }
} }
pagecache->blocks_changed--; pagecache->blocks_changed--;
...@@ -3498,7 +3527,7 @@ static my_bool pagecache_delete_internal(PAGECACHE *pagecache, ...@@ -3498,7 +3527,7 @@ static my_bool pagecache_delete_internal(PAGECACHE *pagecache,
/* See NOTE for pagecache_unlock about registering requests. */ /* See NOTE for pagecache_unlock about registering requests. */
free_block(pagecache, block); free_block(pagecache, block);
err: out:
dec_counter_for_resize_op(pagecache); dec_counter_for_resize_op(pagecache);
return error; return error;
} }
...@@ -4087,6 +4116,7 @@ static void free_block(PAGECACHE *pagecache, PAGECACHE_BLOCK_LINK *block) ...@@ -4087,6 +4116,7 @@ static void free_block(PAGECACHE *pagecache, PAGECACHE_BLOCK_LINK *block)
("block: %u hash_link 0x%lx", ("block: %u hash_link 0x%lx",
PCBLOCK_NUMBER(pagecache, block), PCBLOCK_NUMBER(pagecache, block),
(long) block->hash_link)); (long) block->hash_link));
safe_mutex_assert_owner(&pagecache->cache_lock);
if (block->hash_link) if (block->hash_link)
{ {
/* /*
...@@ -4114,6 +4144,8 @@ static void free_block(PAGECACHE *pagecache, PAGECACHE_BLOCK_LINK *block) ...@@ -4114,6 +4144,8 @@ static void free_block(PAGECACHE *pagecache, PAGECACHE_BLOCK_LINK *block)
KEYCACHE_DBUG_PRINT("free_block", KEYCACHE_DBUG_PRINT("free_block",
("block is freed")); ("block is freed"));
unreg_request(pagecache, block, 0); unreg_request(pagecache, block, 0);
DBUG_ASSERT(block->requests == 0);
DBUG_ASSERT(block->next_used != 0);
block->hash_link= NULL; block->hash_link= NULL;
/* Remove the free block from the LRU ring. */ /* Remove the free block from the LRU ring. */
...@@ -4223,7 +4255,6 @@ static int flush_cached_blocks(PAGECACHE *pagecache, ...@@ -4223,7 +4255,6 @@ static int flush_cached_blocks(PAGECACHE *pagecache,
DBUG_PRINT("info", ("block: %u (0x%lx) to be flushed", DBUG_PRINT("info", ("block: %u (0x%lx) to be flushed",
PCBLOCK_NUMBER(pagecache, block), (ulong)block)); PCBLOCK_NUMBER(pagecache, block), (ulong)block));
PCBLOCK_INFO(block); PCBLOCK_INFO(block);
pagecache_pthread_mutex_unlock(&pagecache->cache_lock);
DBUG_PRINT("info", ("block: %u (0x%lx) pins: %u", DBUG_PRINT("info", ("block: %u (0x%lx) pins: %u",
PCBLOCK_NUMBER(pagecache, block), (ulong)block, PCBLOCK_NUMBER(pagecache, block), (ulong)block,
block->pins)); block->pins));
...@@ -4237,6 +4268,7 @@ static int flush_cached_blocks(PAGECACHE *pagecache, ...@@ -4237,6 +4268,7 @@ static int flush_cached_blocks(PAGECACHE *pagecache,
content (see StaleFilePointersInFlush in ma_checkpoint.c). content (see StaleFilePointersInFlush in ma_checkpoint.c).
@todo change argument of functions to be File. @todo change argument of functions to be File.
*/ */
pagecache_pthread_mutex_unlock(&pagecache->cache_lock);
error= pagecache_fwrite(pagecache, &block->hash_link->file, error= pagecache_fwrite(pagecache, &block->hash_link->file,
block->buffer, block->buffer,
block->hash_link->pageno, block->hash_link->pageno,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment