Fix for Bug #36578 Maria: maria-recover may fail to autorepair a table

Fixed also some similar issues in MyISAM. This was not noticed before as MyISAM did a second retry without key cache (which just made the second repair attempty slower)

storage/maria/ha_maria.cc:
  Print information if we retry without quick in case of CHECK TABLE table_name QUICK
  Remove T_QUICK flag when retrying repair, but set T_SAFE_REPAIR to ensure we don't loose any rows
  Remember T_RETRY_WITH_QUICK flag when restoring repair flags
  Don't print 'checking table' if we are not checking table in auto-repair
  Don't use T_QUICK in auto repair (safer)
  Changed parameter of type HA_PARAM &param to HA_PARAM *param
storage/maria/ha_maria.h:
  Changed parameter of type HA_PARAM &param to HA_PARAM *param
storage/maria/ma_check.c:
  Added retry without T_QUICK if there is a problem reading a row in BLOCK_RECORD
storage/myisam/ha_myisam.cc:
  Remove T_QUICK flag when retrying repair, but set T_SAFE_REPAIR to ensure we don't loose any rows
  Remember T_RETRY_WITH_QUICK flag when restoring repair flags

4 files changed, 85 insertions, 52 deletions

diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc
index b82e2be4adf..420cc953529 100644
--- a/storage/maria/ha_maria.cc
+++ b/storage/maria/ha_maria.cc
@@ -1226,15 +1226,20 @@ int ha_maria::repair(THD * thd, HA_CHECK_OPT *check_opt)
                    (check_opt->flags & T_EXTEND ? T_REP : T_REP_BY_SORT));
   param.sort_buffer_length= THDVAR(thd, sort_buffer_size);
   start_records= file->state->records;
-  while ((error= repair(thd, param, 0)) && param.retry_repair)
+  while ((error= repair(thd, &param, 0)) && param.retry_repair)
   {
     param.retry_repair= 0;
     if (test_all_bits(param.testflag,
                       (uint) (T_RETRY_WITHOUT_QUICK | T_QUICK)))
     {
-      param.testflag &= ~T_RETRY_WITHOUT_QUICK;
-      sql_print_information("Retrying repair of: '%s' without quick",
-                            table->s->path.str);
+      param.testflag&= ~(T_RETRY_WITHOUT_QUICK | T_QUICK);
+      /* Ensure we don't loose any rows when retrying without quick */
+      param.testflag|= T_SAFE_REPAIR;
+      if (thd->vio_ok())
+        _ma_check_print_info(&param, "Retrying repair without quick");
+      else
+        sql_print_information("Retrying repair of: '%s' without quick",
+                              table->s->path.str);
       continue;
     }
     param.testflag &= ~T_QUICK;
@@ -1280,9 +1285,9 @@ int ha_maria::zerofill(THD * thd, HA_CHECK_OPT *check_opt)
 int ha_maria::optimize(THD * thd, HA_CHECK_OPT *check_opt)
 {
   int error;
+  HA_CHECK param;
   if (!file)
     return HA_ADMIN_INTERNAL_ERROR;
-  HA_CHECK param;
 
   maria_chk_init(&param);
   param.thd= thd;
@@ -1290,21 +1295,21 @@ int ha_maria::optimize(THD * thd, HA_CHECK_OPT *check_opt)
   param.testflag= (check_opt->flags | T_SILENT | T_FORCE_CREATE |
                    T_REP_BY_SORT | T_STATISTICS | T_SORT_INDEX);
   param.sort_buffer_length= THDVAR(thd, sort_buffer_size);
-  if ((error= repair(thd, param, 1)) && param.retry_repair)
+  if ((error= repair(thd, &param, 1)) && param.retry_repair)
   {
     sql_print_warning("Warning: Optimize table got errno %d on %s.%s, retrying",
                       my_errno, param.db_name, param.table_name);
     param.testflag &= ~T_REP_BY_SORT;
-    error= repair(thd, param, 1);
+    error= repair(thd, &param, 1);
   }
   return error;
 }
 
 
-int ha_maria::repair(THD *thd, HA_CHECK &param, bool do_optimize)
+int ha_maria::repair(THD *thd, HA_CHECK *param, bool do_optimize)
 {
   int error= 0;
-  ulonglong local_testflag= param.testflag;
+  ulonglong local_testflag= param->testflag;
   bool optimize_done= !do_optimize, statistics_done= 0;
   const char *old_proc_info= thd->proc_info;
   char fixed_name[FN_REFLEN];
@@ -1336,20 +1341,20 @@ int ha_maria::repair(THD *thd, HA_CHECK &param, bool do_optimize)
   if (share->base.born_transactional && !share->now_transactional)
     _ma_copy_nontrans_state_information(file);
 
-  param.db_name= table->s->db.str;
-  param.table_name= table->alias;
-  param.tmpfile_createflag= O_RDWR | O_TRUNC;
-  param.using_global_keycache= 1;
-  param.thd= thd;
-  param.tmpdir= &mysql_tmpdir_list;
-  param.out_flag= 0;
+  param->db_name= table->s->db.str;
+  param->table_name= table->alias;
+  param->tmpfile_createflag= O_RDWR | O_TRUNC;
+  param->using_global_keycache= 1;
+  param->thd= thd;
+  param->tmpdir= &mysql_tmpdir_list;
+  param->out_flag= 0;
   strmov(fixed_name, share->open_file_name);
 
   // Don't lock tables if we have used LOCK TABLE
   if (!thd->locked_tables &&
       maria_lock_database(file, table->s->tmp_table ? F_EXTRA_LCK : F_WRLCK))
   {
-    _ma_check_print_error(&param, ER(ER_CANT_LOCK), my_errno);
+    _ma_check_print_error(param, ER(ER_CANT_LOCK), my_errno);
     DBUG_RETURN(HA_ADMIN_FAILED);
   }
 
@@ -1357,19 +1362,19 @@ int ha_maria::repair(THD *thd, HA_CHECK &param, bool do_optimize)
       ((share->data_file_type == BLOCK_RECORD) ?
        (share->state.changed & STATE_NOT_OPTIMIZED_ROWS) :
        (file->state->del || share->state.split != file->state->records)) &&
-      (!(param.testflag & T_QUICK) ||
+      (!(param->testflag & T_QUICK) ||
        (share->state.changed & (STATE_NOT_OPTIMIZED_KEYS |
                                 STATE_NOT_OPTIMIZED_ROWS))))
   {
     ulonglong key_map= ((local_testflag & T_CREATE_MISSING_KEYS) ?
                         maria_get_mask_all_keys_active(share->base.keys) :
                         share->state.key_map);
-    ulonglong save_testflag= param.testflag;
+    ulonglong save_testflag= param->testflag;
     if (maria_test_if_sort_rep(file, file->state->records, key_map, 0) &&
         (local_testflag & T_REP_BY_SORT))
     {
       local_testflag |= T_STATISTICS;
-      param.testflag |= T_STATISTICS;           // We get this for free
+      param->testflag |= T_STATISTICS;           // We get this for free
       statistics_done= 1;
       /* TODO: Remove BLOCK_RECORD test when parallel works with blocks */
       if (THDVAR(thd,repair_threads) > 1 &&
@@ -1379,28 +1384,28 @@ int ha_maria::repair(THD *thd, HA_CHECK &param, bool do_optimize)
         /* TODO: respect maria_repair_threads variable */
         my_snprintf(buf, 40, "Repair with %d threads", my_count_bits(key_map));
         thd_proc_info(thd, buf);
-        param.testflag|= T_REP_PARALLEL;
-        error= maria_repair_parallel(&param, file, fixed_name,
-                                     test(param.testflag & T_QUICK));
+        param->testflag|= T_REP_PARALLEL;
+        error= maria_repair_parallel(param, file, fixed_name,
+                                     test(param->testflag & T_QUICK));
         /* to reset proc_info, as it was pointing to local buffer */
         thd_proc_info(thd, "Repair done");
       }
       else
       {
         thd_proc_info(thd, "Repair by sorting");
-        param.testflag|= T_REP_BY_SORT;
-        error= maria_repair_by_sort(&param, file, fixed_name,
-                                    test(param.testflag & T_QUICK));
+        param->testflag|= T_REP_BY_SORT;
+        error= maria_repair_by_sort(param, file, fixed_name,
+                                    test(param->testflag & T_QUICK));
       }
     }
     else
     {
       thd_proc_info(thd, "Repair with keycache");
-      param.testflag &= ~(T_REP_BY_SORT | T_REP_PARALLEL);
-      error= maria_repair(&param, file, fixed_name,
-                          test(param.testflag & T_QUICK));
+      param->testflag &= ~(T_REP_BY_SORT | T_REP_PARALLEL);
+      error= maria_repair(param, file, fixed_name,
+                          test(param->testflag & T_QUICK));
     }
-    param.testflag= save_testflag;
+    param->testflag= save_testflag | (param->testflag & T_RETRY_WITHOUT_QUICK);
     optimize_done= 1;
   }
   if (!error)
@@ -1410,7 +1415,7 @@ int ha_maria::repair(THD *thd, HA_CHECK &param, bool do_optimize)
     {
       optimize_done= 1;
       thd_proc_info(thd, "Sorting index");
-      error= maria_sort_index(&param, file, fixed_name);
+      error= maria_sort_index(param, file, fixed_name);
     }
     if (!statistics_done && (local_testflag & T_STATISTICS))
     {
@@ -1418,7 +1423,7 @@ int ha_maria::repair(THD *thd, HA_CHECK &param, bool do_optimize)
       {
         optimize_done= 1;
         thd_proc_info(thd, "Analyzing");
-        error= maria_chk_key(&param, file);
+        error= maria_chk_key(param, file);
       }
       else
         local_testflag &= ~T_STATISTICS;        // Don't update statistics
@@ -1440,18 +1445,18 @@ int ha_maria::repair(THD *thd, HA_CHECK &param, bool do_optimize)
     if (file->state != &share->state.state)
       *file->state= share->state.state;
     if (share->base.auto_key)
-      _ma_update_auto_increment_key(&param, file, 1);
+      _ma_update_auto_increment_key(param, file, 1);
     if (optimize_done)
-      error= maria_update_state_info(&param, file,
+      error= maria_update_state_info(param, file,
                                      UPDATE_TIME | UPDATE_OPEN_COUNT |
                                      (local_testflag &
                                       T_STATISTICS ? UPDATE_STAT : 0));
     info(HA_STATUS_NO_LOCK | HA_STATUS_TIME | HA_STATUS_VARIABLE |
          HA_STATUS_CONST);
-    if (rows != file->state->records && !(param.testflag & T_VERY_SILENT))
+    if (rows != file->state->records && !(param->testflag & T_VERY_SILENT))
     {
       char llbuff[22], llbuff2[22];
-      _ma_check_print_warning(&param, "Number of rows changed from %s to %s",
+      _ma_check_print_warning(param, "Number of rows changed from %s to %s",
                               llstr(rows, llbuff),
                               llstr(file->state->records, llbuff2));
       /* Abort if warning was converted to error */
@@ -1463,7 +1468,7 @@ int ha_maria::repair(THD *thd, HA_CHECK &param, bool do_optimize)
   {
     maria_mark_crashed_on_repair(file);
     file->update |= HA_STATE_CHANGED | HA_STATE_ROW_CHANGED;
-    maria_update_state_info(&param, file, 0);
+    maria_update_state_info(param, file, 0);
   }
   pthread_mutex_unlock(&share->intern_lock);
   thd_proc_info(thd, old_proc_info);
@@ -1471,7 +1476,7 @@ int ha_maria::repair(THD *thd, HA_CHECK &param, bool do_optimize)
     maria_lock_database(file, F_UNLCK);
   error= error ? HA_ADMIN_FAILED :
     (optimize_done ?
-     (write_log_record_for_repair(&param, file) ? HA_ADMIN_FAILED :
+     (write_log_record_for_repair(param, file) ? HA_ADMIN_FAILED :
       HA_ADMIN_OK) : HA_ADMIN_ALREADY_DONE);
   DBUG_RETURN(error);
 }
@@ -1701,15 +1706,16 @@ int ha_maria::enable_indexes(uint mode)
     param.sort_buffer_length= THDVAR(thd,sort_buffer_size);
     param.stats_method= (enum_handler_stats_method)THDVAR(thd,stats_method);
     param.tmpdir= &mysql_tmpdir_list;
-    if ((error= (repair(thd, param, 0) != HA_ADMIN_OK)) && param.retry_repair)
+    if ((error= (repair(thd, &param, 0) != HA_ADMIN_OK)) && param.retry_repair)
     {
-      sql_print_warning("Warning: Enabling keys got errno %d on %s.%s, retrying",
+      sql_print_warning("Warning: Enabling keys got errno %d on %s.%s, "
+                        "retrying",
                         my_errno, param.db_name, param.table_name);
       /* This should never fail normally */
       DBUG_ASSERT(0);
       /* Repairing by sort failed. Now try standard repair method. */
       param.testflag &= ~(T_REP_BY_SORT | T_QUICK);
-      error= (repair(thd, param, 0) != HA_ADMIN_OK);
+      error= (repair(thd, &param, 0) != HA_ADMIN_OK);
       /*
         If the standard repair succeeded, clear all error messages which
         might have been set by the first repair. They can still be seen
@@ -1875,8 +1881,7 @@ end:
 
 bool ha_maria::check_and_repair(THD *thd)
 {
-  int error;
-  int marked_crashed;
+  int error, crashed;
   char *old_query;
   uint old_query_length;
   HA_CHECK_OPT check_opt;
@@ -1905,7 +1910,6 @@ bool ha_maria::check_and_repair(THD *thd)
   // Don't use quick if deleted rows
   if (!file->state->del && (maria_recover_options & HA_RECOVER_QUICK))
     check_opt.flags |= T_QUICK;
-  sql_print_warning("Checking table:   '%s'", table->s->path.str);
 
   old_query= thd->query;
   old_query_length= thd->query_length;
@@ -1914,12 +1918,17 @@ bool ha_maria::check_and_repair(THD *thd)
   thd->query_length= table->s->table_name.length;
   pthread_mutex_unlock(&LOCK_thread_count);
 
-  if ((marked_crashed= maria_is_crashed(file)) || check(thd, &check_opt))
+  if (!(crashed= maria_is_crashed(file)))
+  {
+    sql_print_warning("Checking table:   '%s'", table->s->path.str);
+    crashed= check(thd, &check_opt);
+  }
+
+  if (crashed)
   {
     sql_print_warning("Recovering table: '%s'", table->s->path.str);
     check_opt.flags=
       ((maria_recover_options & HA_RECOVER_BACKUP ? T_BACKUP_DATA : 0) |
-       (marked_crashed ? 0 : T_QUICK) |
        (maria_recover_options & HA_RECOVER_FORCE ? 0 : T_SAFE_REPAIR) |
        T_AUTO_REPAIR);
     if (repair(thd, &check_opt))
diff --git a/storage/maria/ha_maria.h b/storage/maria/ha_maria.h
index 7cb506d89f9..b6c3fea19f2 100644
--- a/storage/maria/ha_maria.h
+++ b/storage/maria/ha_maria.h
@@ -45,7 +45,7 @@ class ha_maria :public handler
     UNDO_BULK_INSERT with/without repair. 
   */
   uint8 bulk_insert_single_undo;
-  int repair(THD * thd, HA_CHECK &param, bool optimize);
+  int repair(THD * thd, HA_CHECK *param, bool optimize);
   int zerofill(THD * thd, HA_CHECK_OPT *check_opt);
 
 public:
diff --git a/storage/maria/ma_check.c b/storage/maria/ma_check.c
index d91a1522055..cf347ebb121 100644
--- a/storage/maria/ma_check.c
+++ b/storage/maria/ma_check.c
@@ -99,6 +99,7 @@ static my_bool _ma_flush_table_files_before_swap(HA_CHECK *param,
                                                  MARIA_HA *info);
 static TrID max_trid_in_system(void);
 static void _ma_check_print_not_visible_error(HA_CHECK *param, TrID used_trid);
+void retry_if_quick(MARIA_SORT_PARAM *param, int error);
 
 
 /* Initialize check param with default values */
@@ -4632,9 +4633,12 @@ static int sort_get_next_record(MARIA_SORT_PARAM *sort_param)
       }
       /* Retry only if wrong record, not if disk error */
       if (flag != HA_ERR_WRONG_IN_RECORD)
+      {
+        retry_if_quick(sort_param, flag);
         DBUG_RETURN(flag);
+      }
     }
-    break;
+    break;                                      /* Impossible */
   }
   case STATIC_RECORD:
     for (;;)
@@ -4644,8 +4648,7 @@ static int sort_get_next_record(MARIA_SORT_PARAM *sort_param)
       {
 	if (sort_param->read_cache.error)
 	  param->out_flag |= O_DATA_LOST;
-        param->retry_repair=1;
-        param->testflag|=T_RETRY_WITHOUT_QUICK;
+        retry_if_quick(sort_param, my_errno);
 	DBUG_RETURN(-1);
       }
       sort_param->start_recpos=sort_param->pos;
@@ -6634,3 +6637,22 @@ static void _ma_check_print_not_visible_error(HA_CHECK *param, TrID used_trid)
     }
   }
 }
+
+
+/**
+  Mark that we can retry normal repair if we used quick repair
+
+  We shouldn't do this in case of disk error as in this case we are likely
+  to loose much more than expected.
+*/
+
+void retry_if_quick(MARIA_SORT_PARAM *sort_param, int error)
+{
+  HA_CHECK *param=sort_param->sort_info->param;
+
+  if (!sort_param->fix_datafile && error >= HA_ERR_FIRST)
+  {
+    param->retry_repair=1;
+    param->testflag|=T_RETRY_WITHOUT_QUICK;
+  }
+}
diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc
index 8c8f3e6495d..ee453d34b75 100644
--- a/storage/myisam/ha_myisam.cc
+++ b/storage/myisam/ha_myisam.cc
@@ -996,7 +996,9 @@ int ha_myisam::repair(THD* thd, HA_CHECK_OPT *check_opt)
     if (test_all_bits(param.testflag,
 		      (uint) (T_RETRY_WITHOUT_QUICK | T_QUICK)))
     {
-      param.testflag&= ~T_RETRY_WITHOUT_QUICK;
+      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",
                             table->s->path.str);
       continue;
@@ -1130,7 +1132,7 @@ int ha_myisam::repair(THD *thd, HA_CHECK &param, bool do_optimize)
       error=  mi_repair(&param, file, fixed_name,
 			test(param.testflag & T_QUICK));
     }
-    param.testflag=testflag;
+    param.testflag= testflag | (param.testflag & T_RETRY_WITHOUT_QUICK);
     optimize_done=1;
   }
   if (!error)