Add phase information to backup_stem to simplify implementation

Author: mariadb-andrzejjarzabek

sql/handler.h

@@ -1519,9 +1519,9 @@ struct backup_target
 
 /** BACKUP SERVER processing phase. Phases must occur in this order. */
 enum backup_proc_phase {
-  BACKUP_PROC_PHASE_BACKUP_LOCKED,  /* concurrent backup guaranteed not to run */
-  BACKUP_PROC_PHASE_DDL_LOCKED,     /* DDL can't happen concurrently */
-  BACKUP_PROC_PHASE_COMMIT_LOCKED}; /* Commits are blocked */
+  BACKUP_PROC_PHASE_BACKUP_LOCKED = 0,  /* concurrent backup guaranteed not to run */
+  BACKUP_PROC_PHASE_DDL_LOCKED,         /* DDL can't happen concurrently */
+  BACKUP_PROC_PHASE_COMMIT_LOCKED};     /* Commits are blocked */
 
 /** BACKUP SERVER ending phase. Phases must occur in this order. */
 enum backup_end_phase {
@@ -1952,10 +1952,11 @@ struct handlerton : public transaction_participant
      Process a unit (file, table etc.) in current phase. Engines should
      define their units in such a way that they may be processed concurrently.
      @param thd   current session
+     @param phase the processing phase for this step
      @return number of units remaining in processing phase, or negative on error
      @retval 0 on completion
   */
-  int (*backup_step)(THD *thd);
+  int (*backup_step)(THD *thd, backup_proc_phase phase);
   /**
      Finish copying and determine the logical time of the backup snapshot.
      @param thd   current sesssion

sql/sql_backup.cc

@@ -217,12 +217,12 @@ static my_bool backup_end(THD *thd, plugin_ref plugin, void *arg) noexcept
   return false;
 }
 
-static my_bool backup_step(THD *thd, plugin_ref plugin, void *) noexcept
+static my_bool backup_step(THD *thd, plugin_ref plugin, void *arg) noexcept
 {
   handlerton *hton= plugin_hton(plugin);
   int res= 0;
   if (hton->backup_step)
-    while ((res= hton->backup_step(thd)))
+    while ((res= hton->backup_step(thd, *static_cast<backup_proc_phase*>(arg))))
       if (res < 0)
         break;
   return res != 0;
@@ -244,13 +244,14 @@ static bool start_proc_phase(THD *thd, backup_proc_phase phase)
                                   &phase);
 }
 
-static bool run_backup_processing(THD *thd)
+static bool run_backup_processing(THD *thd, backup_proc_phase phase)
 {
   /* The backup_step may be invoked in multiple concurrent threads.
   At the time backup_end is invoked, all backup_step will have to complete. */
   return plugin_foreach_with_mask(thd, backup_step,
                                   MYSQL_STORAGE_ENGINE_PLUGIN,
-                                  PLUGIN_IS_DELETED|PLUGIN_IS_READY, nullptr);
+                                  PLUGIN_IS_DELETED|PLUGIN_IS_READY,
+                                  &phase);
 }
 
 static bool run_backup_end_phase(THD *thd, backup_end_phase phase, bool failed)
@@ -271,7 +272,7 @@ static bool upgrade_and_process(THD *thd,
                                                   lock_level,
                                                   thd->variables.lock_wait_timeout) ||
              start_proc_phase(thd, phase);
-  return fail || run_backup_processing(thd);
+  return fail || run_backup_processing(thd, phase);
 }
 
 bool Sql_cmd_backup::execute(THD *thd)
@@ -323,7 +324,7 @@ bool Sql_cmd_backup::execute(THD *thd)
   if (!fail)
   {
     fail= start_proc_phase(thd, BACKUP_PROC_PHASE_BACKUP_LOCKED) ||
-          run_backup_processing(thd) ||
+          run_backup_processing(thd, BACKUP_PROC_PHASE_BACKUP_LOCKED) ||
           upgrade_and_process(thd, mdl_request, MDL_BACKUP_WAIT_DDL,
                               BACKUP_PROC_PHASE_DDL_LOCKED) ||
           upgrade_and_process(thd, mdl_request, MDL_BACKUP_WAIT_COMMIT,

storage/innobase/handler/backup_innodb.cc

@@ -54,8 +54,6 @@ class InnoDB_backup
   backup_context &context() const noexcept
   { ut_ad(log_sys.latch_have_any()); ut_ad(trx); return trx->lock.backup; }
 
-  /** Calls to step() should be ignored */
-  bool ignore_step=false;
 public:
   /**
      Start of BACKUP SERVER: collect all files to be backed up
@@ -141,13 +139,6 @@ class InnoDB_backup
     return fail;
   }
 
-  void stop_processing() noexcept
-  {
-    log_sys.latch.wr_lock();
-    ignore_step= true;
-    log_sys.latch.wr_unlock();
-  }
-
   /**
      Process a file that was collected at init().
      This may be invoked from multiple concurrent threads.
@@ -162,11 +153,6 @@ class InnoDB_backup
     log_sys.latch.wr_lock();
     backup_context &ctx{context()};
     ut_ad(ctx.max_first_lsn);
-    if (ignore_step)
-    {
-      log_sys.latch.wr_unlock();
-      return 0;
-    }
     size_t size{queue.size()};
     if (!logs.empty())
     {
@@ -786,22 +772,13 @@ int innodb_backup_start(THD *thd, backup_target target) noexcept
   return innodb_backup.init(thd, target);
 }
 
-int innodb_backup_start_proc_phase(THD *thd, backup_proc_phase phase) noexcept
+int innodb_backup_step(THD *thd, backup_proc_phase phase) noexcept
 {
-  /* This logic relies of the fact that Backup-locked is the first
-  processing phase and innodb_backup is initially set to process steps.
-  If this should change this code may need to be adjusted, possibly
-  adding a start_processing() method. */
-  if (phase!=BACKUP_PROC_PHASE_BACKUP_LOCKED)
-    innodb_backup.stop_processing();
+  if (phase==BACKUP_PROC_PHASE_BACKUP_LOCKED)
+    return innodb_backup.step(thd);
   return 0;
 }
 
-int innodb_backup_step(THD *thd) noexcept
-{
-  return innodb_backup.step(thd);
-}
-
 int innodb_backup_end(THD *thd, backup_end_phase phase, bool abort) noexcept
 {
   if (phase==BACKUP_END_PHASE_BACKUP_LOCKED)

storage/innobase/handler/backup_innodb.h

@@ -22,22 +22,14 @@
 */
 int innodb_backup_start(THD *thd, backup_target target) noexcept;
 
-/**
-   Start backup processing phase
-   @param thd     current session
-   @param phase   the phase which is started
-   @return error code
-   @retval 0 on success
-*/
-int innodb_backup_start_proc_phase(THD *thd, backup_proc_phase phase) noexcept;
-
 /**
    Process a file that was collected in backup_start().
    @param thd   current session
+   @param phase current processing phase
    @return number of files remaining, or negative on error
    @retval 0 on completion
 */
-int innodb_backup_step(THD *thd) noexcept;
+int innodb_backup_step(THD *thd, backup_proc_phase phase) noexcept;
 
 /**
    Finish copying and determine the logical time of the backup snapshot.

storage/innobase/handler/ha_innodb.cc

@@ -4144,7 +4144,6 @@ static int innodb_init(void* p)
 
         innobase_hton->update_optimizer_costs= innobase_update_optimizer_costs;
 	innobase_hton->backup_start = innodb_backup_start;
-	innobase_hton->backup_start_proc_phase = innodb_backup_start_proc_phase;
 	innobase_hton->backup_step = innodb_backup_step;
 	innobase_hton->backup_end = innodb_backup_end;
 	innobase_hton->backup_finalize = innodb_backup_finalize;

storage/maria/ma_backup.cc

@@ -114,7 +114,6 @@ namespace
 
     bool start_copy_dml_safe() noexcept
     {
-      ddl_locked= true;
       if (scan_dbdirs())
         return true;
       flatten_table_lists();
@@ -129,13 +128,49 @@ namespace
       return false;
     }
 
-    int step() noexcept
+    int dml_safe_copy_step() noexcept
     {
-      if (!ddl_locked)
+      if (dml_safe_tables_copied == dml_safe_table_list.size())
         return 0;
-      return unsafe_copy_phase?
-        unsafe_copy_step():
-        dml_safe_copy_step();
+      const table_ref& table = dml_safe_table_list[dml_safe_tables_copied];
+      if (copy_table(table) != 0)
+        return -1;
+      ++dml_safe_tables_copied;
+      return dml_safe_table_list.size() - dml_safe_tables_copied;
+    }
+
+    int unsafe_copy_step() noexcept
+    {
+      if (have_control_file)
+      {
+        if (copy_control_file() != 0)
+          return -1;
+        have_control_file = false;
+      }
+      else if(log_files_copied < log_files.size())
+      {
+        if (copy_file(log_files[log_files_copied]) != 0)
+          return -1;
+        ++log_files_copied;
+      }
+      else if (unsafe_tables_copied < unsafe_tables_list.size())
+      {
+        const table_ref& table = unsafe_tables_list[unsafe_tables_copied];
+        if (copy_table(table) != 0)
+          return -1;
+        ++unsafe_tables_copied;
+      }
+      else if (misc_files_copied < misc_files.size())
+      {
+        if (copy_file(misc_files[misc_files_copied]) != 0)
+          return -1;
+        ++misc_files_copied;
+      }
+      assert(!have_control_file);
+      return
+        log_files.size() - log_files_copied +
+        unsafe_tables_list.size() - unsafe_tables_copied +
+        misc_files.size() - misc_files_copied;
     }
 
     int end(bool /*abort*/) noexcept
@@ -186,7 +221,6 @@ namespace
     size_t unsafe_tables_copied= 0;
     size_t log_files_copied= 0;
     size_t misc_files_copied= 0;
-    bool ddl_locked= false;
     bool unsafe_copy_phase = false;
 
     int scan_dbdirs() noexcept
@@ -382,51 +416,6 @@ namespace
       return result;
     }
 
-    int dml_safe_copy_step() noexcept
-    {
-      if (dml_safe_tables_copied == dml_safe_table_list.size())
-        return 0;
-      const table_ref& table = dml_safe_table_list[dml_safe_tables_copied];
-      if (copy_table(table) != 0)
-        return -1;
-      ++dml_safe_tables_copied;
-      return dml_safe_table_list.size() - dml_safe_tables_copied;
-    }
-
-    int unsafe_copy_step() noexcept
-    {
-      if (have_control_file)
-      {
-        if (copy_control_file() != 0)
-          return -1;
-        have_control_file = false;
-      }
-      else if(log_files_copied < log_files.size())
-      {
-        if (copy_file(log_files[log_files_copied]) != 0)
-          return -1;
-        ++log_files_copied;
-      }
-      else if (unsafe_tables_copied < unsafe_tables_list.size())
-      {
-        const table_ref& table = unsafe_tables_list[unsafe_tables_copied];
-        if (copy_table(table) != 0)
-          return -1;
-        ++unsafe_tables_copied;
-      }
-      else if (misc_files_copied < misc_files.size())
-      {
-        if (copy_file(misc_files[misc_files_copied]) != 0)
-          return -1;
-        ++misc_files_copied;
-      }
-      assert(!have_control_file);
-      return
-        log_files.size() - log_files_copied +
-        unsafe_tables_list.size() - unsafe_tables_copied +
-        misc_files.size() - misc_files_copied;
-    }
-
     int copy_table(const table_ref& table) noexcept
     {
       dir_ref dir_name = table.first;
@@ -567,9 +556,17 @@ int aria_backup_start_proc_phase(THD *thd, backup_proc_phase phase) noexcept
   }
 }
 
-int aria_backup_step(THD *thd) noexcept
+int aria_backup_step(THD *thd, backup_proc_phase phase) noexcept
 {
-  return aria_backup->step();
+  switch (phase)
+  {
+  case BACKUP_PROC_PHASE_DDL_LOCKED:
+    return aria_backup->dml_safe_copy_step();
+  case BACKUP_PROC_PHASE_COMMIT_LOCKED:
+    return aria_backup->unsafe_copy_step();
+  default:
+    return 0;
+  }
 }
 
 int aria_backup_end(THD *thd, backup_end_phase phase, bool abort) noexcept

storage/maria/ma_backup.h

@@ -39,12 +39,13 @@ int aria_backup_start(THD *thd, backup_target target) noexcept;
 int aria_backup_start_proc_phase(THD *thd, backup_proc_phase phase) noexcept;
 
 /**
-   Process a file that was collected in backup_start().
+   Processing step (processes 1 table).
    @param thd   current session
+   @param phase current processing phase
    @return number of files remaining, or negative on error
    @retval 0 on completion
 */
-int aria_backup_step(THD *thd) noexcept;
+int aria_backup_step(THD *thd, backup_proc_phase phase) noexcept;
 
 /**
    Finish copying and determine the logical time of the backup snapshot.