Commit c9bb8999 authored by Guilhem Bichot's avatar Guilhem Bichot

Fix for BUG#39363 "Concurent inserts in the same table lead to hang in maria engine"

(need a mutex when modifying bitmap->non_flushable), which I hit when running maria_bulk_insert.yy.
After fixing this, I hit an assertion in check_and_set_lsn() saying that the page was PAGECACHE_PLAIN_PAGE.
This could be caused by pages left by an operation which had transactions disabled (like a bulk insert with repair):
in this patch we remove those pages out of the cache when we re-enable transactions.
After fixing this, I get page cache deadlocks, pushbuild2 also has some, to be looked at.
No testcase, requires concurrency and running for 15 minutes, but automatically tested by pushbuild2.


storage/maria/ma_bitmap.c:
  Doing bitmap->non_flushable++ without mutex was wrong. If this ++ happened while another ++ or -- was happening
  in another thread, one ++ or -- could be missed and the bitmap code would behave wrongly. For example, if a ++
  was missed, the DBUG_ASSERT(((int) (bitmap->non_flushable)) >= 0) in _ma_bitmap_release_unused() could fire.
  I saw this assertion happen in practice in maria_bulk_insert.yy. Adding this mutex lock eliminated
  the assertion problem.
  The >=0 was wrong, should be >0 (or the variable could go negative).
storage/maria/ma_recovery.c:
  When we re-enable transactionality, as we may have created pages of type PAGECACHE_PLAIN_PAGE before,
  we need to remove them from the cache (FLUSH_RELEASE). Or they would stay this way, and later when we
  maria_write() to them, we would try to tag them with a LSN (ma_unpin_all_pages()), which is incorrect
  for a plain page (and causes assertion in the page cache at start of check_and_set_lsn()).
  I saw the assertion fire with maria_bulk_insert.yy, and this seems to cure it.
  page cache
parent b7d69564
...@@ -2168,8 +2168,8 @@ void _ma_bitmap_flushable(MARIA_HA *info, int non_flushable_inc) ...@@ -2168,8 +2168,8 @@ void _ma_bitmap_flushable(MARIA_HA *info, int non_flushable_inc)
} }
DBUG_ASSERT(non_flushable_inc == 1); DBUG_ASSERT(non_flushable_inc == 1);
DBUG_ASSERT(info->non_flushable_state == 0); DBUG_ASSERT(info->non_flushable_state == 0);
/* It is a read without mutex because only an optimization */ pthread_mutex_lock(&bitmap->bitmap_lock);
if (unlikely(bitmap->flush_all_requested)) while (unlikely(bitmap->flush_all_requested))
{ {
/* /*
Some other thread is waiting for the bitmap to become Some other thread is waiting for the bitmap to become
...@@ -2182,21 +2182,13 @@ void _ma_bitmap_flushable(MARIA_HA *info, int non_flushable_inc) ...@@ -2182,21 +2182,13 @@ void _ma_bitmap_flushable(MARIA_HA *info, int non_flushable_inc)
our thread), it is not going to increase it more so is not going to come our thread), it is not going to increase it more so is not going to come
here. here.
*/ */
pthread_mutex_lock(&bitmap->bitmap_lock); DBUG_PRINT("info", ("waiting for bitmap flusher"));
while (bitmap->flush_all_requested) pthread_cond_wait(&bitmap->bitmap_cond, &bitmap->bitmap_lock);
{
DBUG_PRINT("info", ("waiting for bitmap flusher"));
pthread_cond_wait(&bitmap->bitmap_cond, &bitmap->bitmap_lock);
}
pthread_mutex_unlock(&bitmap->bitmap_lock);
} }
/*
Ok to set without mutex: we didn't touch the bitmap's content yet; when we
touch it we will take the mutex.
*/
bitmap->non_flushable++; bitmap->non_flushable++;
info->non_flushable_state= 1; info->non_flushable_state= 1;
DBUG_PRINT("info", ("bitmap->non_flushable: %u", bitmap->non_flushable)); DBUG_PRINT("info", ("bitmap->non_flushable: %u", bitmap->non_flushable));
pthread_mutex_unlock(&bitmap->bitmap_lock);
DBUG_VOID_RETURN; DBUG_VOID_RETURN;
} }
...@@ -2308,7 +2300,7 @@ my_bool _ma_bitmap_release_unused(MARIA_HA *info, MARIA_BITMAP_BLOCKS *blocks) ...@@ -2308,7 +2300,7 @@ my_bool _ma_bitmap_release_unused(MARIA_HA *info, MARIA_BITMAP_BLOCKS *blocks)
/* This duplicates ma_bitmap_flushable(-1) except it already has mutex */ /* This duplicates ma_bitmap_flushable(-1) except it already has mutex */
if (info->non_flushable_state) if (info->non_flushable_state)
{ {
DBUG_ASSERT((int) bitmap->non_flushable >= 0); DBUG_ASSERT(((int) (bitmap->non_flushable)) > 0);
info->non_flushable_state= 0; info->non_flushable_state= 0;
if (--bitmap->non_flushable == 0) if (--bitmap->non_flushable == 0)
{ {
......
...@@ -3292,13 +3292,13 @@ my_bool _ma_reenable_logging_for_table(MARIA_HA *info, my_bool flush_pages) ...@@ -3292,13 +3292,13 @@ my_bool _ma_reenable_logging_for_table(MARIA_HA *info, my_bool flush_pages)
/* /*
We are going to change callbacks; if a page is flushed at this moment We are going to change callbacks; if a page is flushed at this moment
this can cause race conditions, that's one reason to flush pages this can cause race conditions, that's one reason to flush pages
now. Other reasons: a checkpoint could be running and miss pages. As now. Other reasons: a checkpoint could be running and miss pages; the
pages have type PAGECACHE_PLAIN_PAGE which should not remain. As
there are no REDOs for pages, them, bitmaps and the state also have to there are no REDOs for pages, them, bitmaps and the state also have to
be flushed and synced. Leaving non-dirty pages in cache is ok, when be flushed and synced.
they become dirty again they will have their type corrected.
*/ */
if (_ma_flush_table_files(info, MARIA_FLUSH_DATA | MARIA_FLUSH_INDEX, if (_ma_flush_table_files(info, MARIA_FLUSH_DATA | MARIA_FLUSH_INDEX,
FLUSH_KEEP, FLUSH_KEEP) || FLUSH_RELEASE, FLUSH_RELEASE) ||
_ma_state_info_write(share, 1|4) || _ma_state_info_write(share, 1|4) ||
_ma_sync_table_files(info)) _ma_sync_table_files(info))
DBUG_RETURN(1); DBUG_RETURN(1);
......
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