Commit 8b87da7f 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 edf84296
......@@ -2168,8 +2168,8 @@ void _ma_bitmap_flushable(MARIA_HA *info, int non_flushable_inc)
}
DBUG_ASSERT(non_flushable_inc == 1);
DBUG_ASSERT(info->non_flushable_state == 0);
/* It is a read without mutex because only an optimization */
if (unlikely(bitmap->flush_all_requested))
pthread_mutex_lock(&bitmap->bitmap_lock);
while (unlikely(bitmap->flush_all_requested))
{
/*
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)
our thread), it is not going to increase it more so is not going to come
here.
*/
pthread_mutex_lock(&bitmap->bitmap_lock);
while (bitmap->flush_all_requested)
{
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++;
info->non_flushable_state= 1;
DBUG_PRINT("info", ("bitmap->non_flushable: %u", bitmap->non_flushable));
pthread_mutex_unlock(&bitmap->bitmap_lock);
DBUG_VOID_RETURN;
}
......@@ -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 */
if (info->non_flushable_state)
{
DBUG_ASSERT((int) bitmap->non_flushable >= 0);
DBUG_ASSERT(((int) (bitmap->non_flushable)) > 0);
info->non_flushable_state= 0;
if (--bitmap->non_flushable == 0)
{
......
......@@ -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
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
be flushed and synced. Leaving non-dirty pages in cache is ok, when
they become dirty again they will have their type corrected.
be flushed and synced.
*/
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_sync_table_files(info))
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