bzr commit into MySQL/Maria:mysql-maria branch (guilhem:2682) Bug#39363

[Guilhem Bichot]

#At bzr+ssh://bk-internal.mysql.com/bzrroot/server/mysql-maria/

 2682 Guilhem Bichot	2008-10-17
      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.
modified:
  storage/maria/ma_bitmap.c
  storage/maria/ma_recovery.c

per-file messages:
  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
=== modified file 'storage/maria/ma_bitmap.c'
--- a/storage/maria/ma_bitmap.c	2008-10-14 15:18:14 +0000
+++ b/storage/maria/ma_bitmap.c	2008-10-17 13:37:07 +0000
@@ -2168,8 +2168,8 @@ void _ma_bitmap_flushable(MARIA_HA *info
   }
   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
       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);
+    DBUG_PRINT("info", ("waiting for bitmap flusher"));
+    pthread_cond_wait(&bitmap->bitmap_cond, &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_
   /* 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)
     {

=== modified file 'storage/maria/ma_recovery.c'
--- a/storage/maria/ma_recovery.c	2008-08-25 11:49:47 +0000
+++ b/storage/maria/ma_recovery.c	2008-10-17 13:37:07 +0000
@@ -3292,13 +3292,13 @@ my_bool _ma_reenable_logging_for_table(M
       /*
         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);