Fixed several errors in Aria discovered by test case for lp:727869...

Fixed several errors in Aria discovered by test case for lp:727869 ma_pagecache.c:2103: find_block: Assertion `block->rlocks == 0
- Fixed assert in transaction log handler when aria_check was run on block-record table that was much bigger than expected.
- Fixed warnings about wrong mutex order between bitmap and intern_lock
- Fixed error in bitmap that could cause two rows to use same block for a block record.
- Fixed wrong test that could cause error if last page for a bitmap was used by a blob.
- Fixed several bugs in pagecache for the case where pagecase had very few blocks and there was a lot of threads competing to get the blocks (very unlikely case).


mysql-test/suite/maria/r/maria-recovery3.result:
  Updated results
sql/mysqld.cc:
  Allow mi_check() to send information messages for log file
storage/maria/ma_bitmap.c:
  Fixed problem with wrong mutex order when bitmap was the first page that was flushed out of page cache
  - Fixed by introducing _ma_bitmap_mark_file_changed() that marks file changed without a bitmap lock.
  - Fixed one case in _ma_change_bitmap_page() where we didn't mark the bitmap changed. This could cause to rows to reuse same block if this was the only change to the bitmap.
  - Split _ma_bitmap_get_page_bits() in two parts to not take a bitmap lock when we already have it
  - Fixed bug in _ma_bitmap_set_full_page_bits() that caused an error if last page for a bitmap was used by a blob
storage/maria/ma_check.c:
  Better handling of wrong file length.
  Fixed bug when we tried to write to transaction log when it was not opened (happened when block record file was bigger than expected)
storage/maria/ma_pagecache.c:
  Fixed several bugs in pagecache for the case where pagecase had very few blocks and there was a lot of threads competing to get the blocks:
  - In link_block() mark a block given to another thread with PCBLOCK_REASSIGNED to ensure that no other threads can start re-using the block
    before the thread that requsted a block.
  - In free_block(), don't reset status for a block that is in re-assign by link_block() (we don't want to loose the PCBLOCK_REASSIGNED flag).
  - Added call to wait_for_flush() when we got a new block in find_block() to ensure that we don't use a block that is beeing flushed by another thread.
  - Moved setting of hits_left and last_hit_time in find_block() to where we assign the block.
  
  
  Code cleanup and making code uniform:
  - Changed a lot of KEYCACHE_DBUG_PRINT to use DBUG_PRINT
  - Streamlined all reporting of 'signal' and 'wait' between threads to be identical.
  - Use thread name instead of thread number (for each match against --debug)
  - Added more DBUG_ENTER, DBUG_PRINT and DBUG_ASSERT()
  - Added more comments
storage/myisam/ha_myisam.cc:
  Only print information about that we make a backup if we are really making a backup.
storage/myisam/mi_check.c:
  Inform mysqld that we are creating a backup of the data file (for inclusion in error log).

mysql-test/suite/maria/r/maria-recovery3.result

@@ -78,7 +78,7 @@ ERROR HY000: Lost connection to MySQL server during query
 * recovery happens
 check table t1 extended;
 Table	Op	Msg_type	Msg_text
-mysqltest.t1	check	warning	Size of indexfile is: 372      Should be: 8192
+mysqltest.t1	check	warning	Size of indexfile is: 372       Expected: 8192
 mysqltest.t1	check	status	OK
 * testing that checksum after recovery is as expected
 Checksum-check

sql/mysqld.cc

@@ -3048,7 +3048,8 @@ int my_message_sql(uint error, const char *str, myf MyFlags)
   {
     /* At least, prevent new abuse ... */
     DBUG_ASSERT(strncmp(str, "MyISAM table", 12) == 0 ||
-                strncmp(str, "Aria table", 11) == 0);
+                strncmp(str, "Aria table", 11) == 0 ||
+                (MyFlags & ME_JUST_INFO));
     error= ER_UNKNOWN_ERROR;
   }
 

storage/maria/ma_bitmap.c

@@ -310,6 +310,31 @@ my_bool _ma_bitmap_end(MARIA_SHARE *share)
   return res;
 }
 
+/*
+  Ensure that we have incremented open count before we try to read/write
+  a page while we have the bitmap lock.
+  This is needed to ensure that we don't call _ma_mark_file_changed() as
+  part of flushing a page to disk, as this locks share->internal_lock
+  and then mutex lock would happen in the wrong order.
+*/
+
+static inline void _ma_bitmap_mark_file_changed(MARIA_SHARE *share)
+{                                                
+  /* 
+     It's extremely unlikely that the following test is true as it
+     only happens once if the table has changed.
+  */
+  if (unlikely(!share->global_changed &&
+               (share->state.changed & STATE_CHANGED)))
+  {
+    /* purecov: begin inspected */
+    /* unlock mutex as it can't be hold during _ma_mark_file_changed() */
+    pthread_mutex_unlock(&share->bitmap.bitmap_lock);
+    _ma_mark_file_changed(share);
+    pthread_mutex_lock(&share->bitmap.bitmap_lock);
+    /* purecov: end */
+  }
+}
 
 /*
   Send updated bitmap to the page cache
@@ -413,24 +438,13 @@ my_bool _ma_bitmap_flush_all(MARIA_SHARE *share)
     DBUG_RETURN(0);
   }
 
+  _ma_bitmap_mark_file_changed(share);
+
   /*
-    Before flusing bitmap, ensure that we have incremented open count.
-    This is needed to ensure that we don't call
-    _ma_mark_file_changed() as part of flushing bitmap page as in this
-    case we would use mutex lock in wrong order.
-    It's extremely unlikely that the following test is true as normally
-    this is happening when table is flushed.
+    The following should be true as it was tested above. We have to test
+    this again as _ma_bitmap_mark_file_changed() did temporarly release
+    the bitmap mutex.
   */
-  if (unlikely(!share->global_changed))
-  {
-    /* purecov: begin inspected */
-    /* unlock bitmap mutex as it can't be hold during _ma_mark_file_changed */
-    pthread_mutex_unlock(&bitmap->bitmap_lock);
-    _ma_mark_file_changed(share);
-    pthread_mutex_lock(&bitmap->bitmap_lock);
-    /* purecov: end */
-  }
-
   if (bitmap->changed || bitmap->changed_not_flushed)
   {
     bitmap->flush_all_requested++;
@@ -1010,6 +1024,8 @@ static my_bool _ma_change_bitmap_page(MARIA_HA *info,
 {
   DBUG_ENTER("_ma_change_bitmap_page");
 
+  _ma_bitmap_mark_file_changed(info->s);
+
   if (bitmap->changed)
   {
     if (write_changed_bitmap(info->s, bitmap))
@@ -1447,10 +1463,7 @@ static ulong allocate_full_pages(MARIA_FILE_BITMAP *bitmap,
     best_prefix_bits|= tmp;
     int6store(best_data, best_prefix_bits);
     if (!(best_area_size-= best_prefix_area_size))
-    {
-      DBUG_EXECUTE("bitmap", _ma_print_bitmap_changes(bitmap););
-      DBUG_RETURN(block->page_count);
-    }
+      goto end;
     best_data+= 6;
   }
   best_area_size*= 3;                       /* Bits to set */
@@ -1468,6 +1481,7 @@ static ulong allocate_full_pages(MARIA_FILE_BITMAP *bitmap,
     bitmap->used_size= (uint) (best_data - bitmap->map);
     DBUG_ASSERT(bitmap->used_size <= bitmap->total_size);
   }
+end:
   bitmap->changed= 1;
   DBUG_EXECUTE("bitmap", _ma_print_bitmap_changes(bitmap););
   DBUG_RETURN(block->page_count);
@@ -2142,7 +2156,7 @@ static my_bool set_page_bits(MARIA_HA *info, MARIA_FILE_BITMAP *bitmap,
   Get bitmap pattern for a given page
 
   SYNOPSIS
-    get_page_bits()
+    bitmap_get_page_bits()
     info	Maria handler
     bitmap	Bitmap handler
     page	Page number
@@ -2152,8 +2166,8 @@ static my_bool set_page_bits(MARIA_HA *info, MARIA_FILE_BITMAP *bitmap,
     ~0		Error (couldn't read page)
 */
 
-uint _ma_bitmap_get_page_bits(MARIA_HA *info, MARIA_FILE_BITMAP *bitmap,
-                              pgcache_page_no_t page)
+static uint bitmap_get_page_bits(MARIA_HA *info, MARIA_FILE_BITMAP *bitmap,
+                                 pgcache_page_no_t page)
 {
   pgcache_page_no_t bitmap_page;
   uint offset_page, offset, tmp;
@@ -2179,6 +2193,19 @@ uint _ma_bitmap_get_page_bits(MARIA_HA *info, MARIA_FILE_BITMAP *bitmap,
 }
 
 
+/* As above, but take a lock while getting the data */
+
+uint _ma_bitmap_get_page_bits(MARIA_HA *info, MARIA_FILE_BITMAP *bitmap,
+                              pgcache_page_no_t page)
+{
+  uint tmp;
+  pthread_mutex_lock(&bitmap->bitmap_lock);
+  tmp= bitmap_get_page_bits(info, bitmap, page);
+  pthread_mutex_unlock(&bitmap->bitmap_lock);  
+  return tmp;
+}
+
+
 /*
   Mark all pages in a region as free
 
@@ -2258,6 +2285,7 @@ my_bool _ma_bitmap_reset_full_page_bits(MARIA_HA *info,
   DBUG_RETURN(0);
 }
 
+
 /*
   Set all pages in a region as used
 
@@ -2290,7 +2318,7 @@ my_bool _ma_bitmap_set_full_page_bits(MARIA_HA *info,
 
   bitmap_page= page - page % bitmap->pages_covered;
   if (page == bitmap_page ||
-      page + page_count >= bitmap_page + bitmap->pages_covered)
+      page + page_count > bitmap_page + bitmap->pages_covered)
   {
     DBUG_ASSERT(0);                             /* Wrong in data */
     DBUG_RETURN(1);
@@ -2494,7 +2522,7 @@ my_bool _ma_bitmap_release_unused(MARIA_HA *info, MARIA_BITMAP_BLOCKS *blocks)
   else
   {
     DBUG_ASSERT(current_bitmap_value ==
-                _ma_bitmap_get_page_bits(info, bitmap, block->page));
+                bitmap_get_page_bits(info, bitmap, block->page));
   }
 
   /* Handle all full pages and tail pages (for head page and blob) */
@@ -2526,7 +2554,7 @@ my_bool _ma_bitmap_release_unused(MARIA_HA *info, MARIA_BITMAP_BLOCKS *blocks)
         to not set the bits to the same value as before.
       */
       DBUG_ASSERT(current_bitmap_value ==
-                  _ma_bitmap_get_page_bits(info, bitmap, block->page));
+                  bitmap_get_page_bits(info, bitmap, block->page));
 
       if (bits != current_bitmap_value)
       {

storage/maria/ma_check.c

@@ -412,12 +412,13 @@ int maria_chk_size(HA_CHECK *param, register MARIA_HA *info)
     {
       error=1;
       _ma_check_print_error(param,
-			   "Size of indexfile is: %-8s        Should be: %s",
+			   "Size of indexfile is: %-8s         Expected: %s",
 			   llstr(size,buff), llstr(skr,buff2));
+      share->state.state.key_file_length= size;
     }
     else if (!(param->testflag & T_VERY_SILENT))
       _ma_check_print_warning(param,
-			     "Size of indexfile is: %-8s      Should be: %s",
+			     "Size of indexfile is: %-8s       Expected: %s",
 			     llstr(size,buff), llstr(skr,buff2));
   }
   if (!(param->testflag & T_VERY_SILENT) &&
@@ -439,18 +440,18 @@ int maria_chk_size(HA_CHECK *param, register MARIA_HA *info)
 #endif
   if (skr != size)
   {
+    share->state.state.data_file_length=size;	/* Skip other errors */
     if (skr > size && skr != size + MEMMAP_EXTRA_MARGIN)
     {
-      share->state.state.data_file_length=size;	/* Skip other errors */
       error=1;
-      _ma_check_print_error(param,"Size of datafile is: %-9s         Should be: %s",
+      _ma_check_print_error(param,"Size of datafile is: %-9s         Expected: %s",
 		    llstr(size,buff), llstr(skr,buff2));
       param->testflag|=T_RETRY_WITHOUT_QUICK;
     }
     else
     {
       _ma_check_print_warning(param,
-                              "Size of datafile is: %-9s       Should be: %s",
+                              "Size of datafile is: %-9s       Expected: %s",
                               llstr(size,buff), llstr(skr,buff2));
     }
   }
@@ -1801,7 +1802,7 @@ static int check_block_record(HA_CHECK *param, MARIA_HA *info, int extend,
   char llbuff[22], llbuff2[22];
   uint block_size= share->block_size;
   ha_rows full_page_count, tail_count;
-  my_bool full_dir;
+  my_bool full_dir, now_transactional;
   uint offset_page, offset, free_count;
 
   LINT_INIT(full_dir);
@@ -1812,6 +1813,10 @@ static int check_block_record(HA_CHECK *param, MARIA_HA *info, int extend,
                           my_errno);
     return 1;
   }
+
+  now_transactional= info->s->now_transactional;
+  info->s->now_transactional= 0;                /* Don't log changes */
+
   bitmap_buff= info->scan.bitmap_buff;
   page_buff= info->scan.page_buff;
   full_page_count= tail_count= 0;
@@ -1831,7 +1836,8 @@ static int check_block_record(HA_CHECK *param, MARIA_HA *info, int extend,
     if (_ma_killed_ptr(param))
     {
       _ma_scan_end_block_record(info);
-      return -1;
+      info->s->now_transactional= now_transactional;
+      return -1;                                /* Interrupted */
     }
     if ((page % share->bitmap.pages_covered) == 0)
     {
@@ -1999,10 +2005,12 @@ static int check_block_record(HA_CHECK *param, MARIA_HA *info, int extend,
                           llstr(param->tail_count, llbuff),
                           llstr(tail_count, llbuff2));
 
+  info->s->now_transactional= now_transactional;
   return param->error_printed != 0;
 
 err:
   _ma_scan_end_block_record(info);
+  info->s->now_transactional= now_transactional;
   return 1;
 }
 

storage/myisam/ha_myisam.cc

@@ -1072,7 +1072,7 @@ int ha_myisam::repair(THD* thd, HA_CHECK_OPT *check_opt)
       param.testflag&= ~(T_RETRY_WITHOUT_QUICK | T_QUICK);
       /* Ensure we don't loose any rows when retrying without quick */
       param.testflag|= T_SAFE_REPAIR;
-      sql_print_information("Retrying repair of: '%s' without quick",
+      sql_print_information("Retrying repair of: '%s' including modifying data file",
                             table->s->path.str);
       continue;
     }
@@ -1666,15 +1666,15 @@ bool ha_myisam::check_and_repair(THD *thd)
   if ((marked_crashed= mi_is_crashed(file)) || check(thd, &check_opt))
   {
     sql_print_warning("Recovering table: '%s'",table->s->path.str);
-    if (myisam_recover_options & (HA_RECOVER_FULL_BACKUP | HA_RECOVER_BACKUP))
+    if (myisam_recover_options & HA_RECOVER_FULL_BACKUP)
     {
       char buff[MY_BACKUP_NAME_EXTRA_LENGTH+1];
       my_create_backup_name(buff, "", check_opt.start_time);
-      sql_print_information("Making backup of data with extension '%s'", buff);
-    }
-    if (myisam_recover_options & HA_RECOVER_FULL_BACKUP)
+      sql_print_information("Making backup of index file with extension '%s'",
+                            buff);
       mi_make_backup_of_index(file, check_opt.start_time,
                               MYF(MY_WME | ME_JUST_WARNING));
+    }
     check_opt.flags=
       (((myisam_recover_options &
          (HA_RECOVER_BACKUP | HA_RECOVER_FULL_BACKUP)) ? T_BACKUP_DATA : 0) |

storage/myisam/mi_check.c

@@ -85,6 +85,8 @@ static SORT_KEY_BLOCKS	*alloc_key_blocks(HA_CHECK *param, uint blocks,
 					  uint buffer_length);
 static ha_checksum mi_byte_checksum(const uchar *buf, uint length);
 static void set_data_file_type(MI_SORT_INFO *sort_info, MYISAM_SHARE *share);
+static int replace_data_file(HA_CHECK *param, MI_INFO *info,
+                             const char *name, File new_file);
 
 void myisamchk_init(HA_CHECK *param)
 {
@@ -1733,17 +1735,8 @@ err:
     /* Replace the actual file with the temporary file */
     if (new_file >= 0)
     {
-      my_close(new_file,MYF(0));
-      info->dfile=new_file= -1;
-      if (change_to_newfile(share->data_file_name,MI_NAME_DEXT,
-			    DATA_TMP_EXT,
-                            param->backup_time,
-                            share->base.raid_chunks,
-			    (param->testflag & T_BACKUP_DATA ?
-			     MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
-	  mi_open_datafile(info,share,name,-1))
-	got_error=1;
-
+      got_error= replace_data_file(param, info, name, new_file);
+      new_file= -1;
       param->retry_repair= 0;
     }
   }
@@ -2553,15 +2546,8 @@ err:
     /* Replace the actual file with the temporary file */
     if (new_file >= 0)
     {
-      my_close(new_file,MYF(0));
-      info->dfile=new_file= -1;
-      if (change_to_newfile(share->data_file_name,MI_NAME_DEXT,
-			    DATA_TMP_EXT, param->backup_time,
-                            share->base.raid_chunks,
-			    (param->testflag & T_BACKUP_DATA ?
-			     MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
-	  mi_open_datafile(info,share,name,-1))
-	got_error=1;
+      got_error= replace_data_file(param, info, name, new_file);
+      new_file= -1;
     }
   }
   if (got_error)
@@ -3092,15 +3078,8 @@ err:
     /* Replace the actual file with the temporary file */
     if (new_file >= 0)
     {
-      my_close(new_file,MYF(0));
-      info->dfile=new_file= -1;
-      if (change_to_newfile(share->data_file_name,MI_NAME_DEXT,
-			    DATA_TMP_EXT, param->backup_time,
-                            share->base.raid_chunks,
-			    (param->testflag & T_BACKUP_DATA ?
-			     MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
-	  mi_open_datafile(info,share,name,-1))
-	got_error=1;
+      got_error= replace_data_file(param, info, name, new_file);
+      new_file= -1;
     }
   }
   if (got_error)
@@ -4765,3 +4744,29 @@ int mi_make_backup_of_index(MI_INFO *info, time_t backup_time, myf flags)
   my_create_backup_name(backup_name, info->s->index_file_name, backup_time);
   return my_copy(info->s->index_file_name, backup_name, flags);
 }
+
+static int replace_data_file(HA_CHECK *param, MI_INFO *info,
+                             const char *name, File new_file)
+{
+  MYISAM_SHARE *share=info->s;
+
+  my_close(new_file,MYF(0));
+  info->dfile= -1;
+  if (param->testflag & T_BACKUP_DATA)
+  {
+    char buff[MY_BACKUP_NAME_EXTRA_LENGTH+1];
+    my_create_backup_name(buff, "", param->backup_time);
+    my_printf_error(0,                          /* No error, just info */
+                    "Making backup of data file with extension '%s'",
+                    MYF(ME_JUST_INFO | ME_NOREFRESH), buff);
+  }
+
+  if (change_to_newfile(share->data_file_name,MI_NAME_DEXT,
+                        DATA_TMP_EXT, param->backup_time,
+                        share->base.raid_chunks,
+                        (param->testflag & T_BACKUP_DATA ?
+                         MYF(MY_REDEL_MAKE_BACKUP): MYF(0))) ||
+      mi_open_datafile(info, share, name, -1))
+    return 1;
+  return 0;
+}