bzr commit into mysql-6.0-backup branch (rsomla:2673) WL#4384

[Rafal Somla]

#At file:///ext/mysql/bzr/backup/wl4384/

 2673 Rafal Somla	2008-07-31
      WL#4384 (Online backup: Revise error reporting)
            
      This patch marks places in the code where errors reported by called functions are
not detected
      or not logged. This is for further analysis and fixing of these issues.
           
      The markers are "// FIXME: detect errors..." and "// FIXME: error logging...".
modified:
  sql/backup/backup_aux.h
  sql/backup/backup_info.cc
  sql/backup/data_backup.cc
  sql/backup/image_info.cc
  sql/backup/image_info.h
  sql/backup/kernel.cc

=== modified file 'sql/backup/backup_aux.h'
--- a/sql/backup/backup_aux.h	2008-07-09 07:12:43 +0000
+++ b/sql/backup/backup_aux.h	2008-07-31 10:45:02 +0000
@@ -1,6 +1,18 @@
 #ifndef _BACKUP_AUX_H
 #define _BACKUP_AUX_H
 
+/** 
+  @file
+ 
+  @brief Auxiliary declarations used in online backup code.
+
+  @todo Fix error detection in places marked with "FIXME: detect errors...". 
+  These are places where functions or methods are called and if they can 
+  report errors it should be detected and appropriate action taken. If callee 
+  never reports errors or we want to ignore errors, a comment explaining this
+  should be added.
+*/ 
+
 typedef st_plugin_int* storage_engine_ref;
 
 // Macro which transforms plugin_ref to storage_engine_ref
@@ -131,6 +143,7 @@ void set_table_list(TABLE_LIST &tl, cons
   tl.db= const_cast<char*>(tbl.db().name().ptr());
   tl.lock_type= lock_type;
 
+  // FIXME: detect errors (if NULL returned).
   tl.mdl_lock_data= mdl_alloc_lock(0, tl.db, tl.table_name, mem); 
 }
 

=== modified file 'sql/backup/backup_info.cc'
--- a/sql/backup/backup_info.cc	2008-07-09 07:12:43 +0000
+++ b/sql/backup/backup_info.cc	2008-07-31 10:45:02 +0000
@@ -4,7 +4,18 @@
   Implementation of @c Backup_info class. Method @c find_backup_engine()
   implements algorithm for selecting backup engine used to backup
   given table.
- */
+  
+  @todo Fix error detection in places marked with "FIXME: detect errors...". 
+  These are places where functions or methods are called and if they can 
+  report errors it should be detected and appropriate action taken. If callee 
+  never reports errors or we want to ignore errors, a comment explaining this
+  should be added.
+
+  @todo Fix error logging in places marked with "FIXME: error logging...". In 
+  these places it should be decided if and how the error should be shown to the
+  user. If an error message should be logged, it can happen either in the place
+  where error was detected or somewhere up the call stack.
+*/
 
 #include "../mysql_priv.h"
 
@@ -306,8 +317,12 @@ Backup_info::Backup_info(Backup_restore_
 
   bzero(m_snap, sizeof(m_snap));
 
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   hash_init(&ts_hash, &::my_charset_bin, 16, 0, 0,
             Ts_hash_node::get_key, Ts_hash_node::free, MYF(0));
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   hash_init(&dep_hash, &::my_charset_bin, 16, 0, 0,
             Dep_node::get_key, Dep_node::free, MYF(0));
 
@@ -319,23 +334,32 @@ Backup_info::Backup_info(Backup_restore_
 
   snap= new Nodata_snapshot(m_ctx);  // reports errors
 
+  // FIXME: error logging (in case snap could not be allocated).
   if (!snap || !snap->is_valid())
     return;
 
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   snapshots.push_back(snap);
 
   snap= new CS_snapshot(m_ctx); // reports errors
 
+  // FIXME: error logging (in case snap could not be allocated).
   if (!snap || !snap->is_valid())
     return;
 
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   snapshots.push_back(snap);
 
   snap= new Default_snapshot(m_ctx);  // reports errors
 
+  // FIXME: error logging (in case snap could not be allocated).
   if (!snap || !snap->is_valid())
     return;
 
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   snapshots.push_back(snap);
 
   m_state= CREATED;
@@ -1240,8 +1264,9 @@ inline
 Backup_info::Global_iterator::Global_iterator(const Image_info &info)
  :Iterator(info), mode(TABLESPACES), m_it(NULL), m_obj(NULL)
 {
+  // FIXME: detect errors (if NULL returned).
   m_it= m_info.get_tablespaces();
-  next();
+  next();       // Note: next() doesn't report errors.
 }
 
 

=== modified file 'sql/backup/data_backup.cc'
--- a/sql/backup/data_backup.cc	2008-07-07 12:51:56 +0000
+++ b/sql/backup/data_backup.cc	2008-07-31 10:45:02 +0000
@@ -7,6 +7,17 @@
   drivers and protocols to create snapshot of the data stored in the tables being
   backed up.
 
+  @todo Fix error detection in places marked with "FIXME: detect errors...". 
+  These are places where functions or methods are called and if they can 
+  report errors it should be detected and appropriate action taken. If callee 
+  never reports errors or we want to ignore errors, a comment explaining this
+  should be added.
+
+  @todo Fix error logging in places marked with "FIXME: error logging...". In 
+  these places it should be decided if and how the error should be shown to the
+  user. If an error message should be logged, it can happen either in the place
+  where error was detected or somewhere up the call stack.
+
   @todo Implement better scheduling strategy in Scheduler::step
   @todo Add error reporting in the scheduler and elsewhere
   @todo If an error from driver is ignored (and operation retried) leave trace
@@ -459,6 +470,8 @@ int write_table_data(THD* thd, Backup_in
       if (init_size > max_init_size)
         max_init_size= init_size;
 
+      // FIXME: detect errors if reported.
+      // FIXME: error logging.
       inactive.push_back(p);
     }
   }
@@ -579,6 +592,8 @@ int write_table_data(THD* thd, Backup_in
       Save binlog information for point in time recovery on restore.
     */
     if (mysql_bin_log.is_open())
+      // FIXME: detect errors if reported.
+      // FIXME: error logging.
       mysql_bin_log.get_current_log(&binlog_pos);
 
     /*
@@ -766,12 +781,14 @@ int Scheduler::step()
       break;
 
     case backup_state::DONE:
+      // FIXME: detect errors if reported.
       p->end();
 
     case backup_state::ERROR:
-      remove_pump(p);
+      remove_pump(p);   // Note: never errors.
       if (res)
         cancel_backup(); // we hit an error - bail out
+                         // Note: cancel_backup() never errors.
       break;
 
     default: break;
@@ -903,8 +920,8 @@ void Scheduler::cancel_backup()
   while (m_count && m_pumps)
   {
     Pump_iterator p(*this);
-    p->cancel();
-    remove_pump(p);
+    p->cancel();        // Note: even if cancel() errors, we ignore it.
+    remove_pump(p);     // Note: never errors.
   }
 
   cancelled= TRUE;
@@ -923,7 +940,7 @@ int Scheduler::prepare()
   {
     if (it->prepare())
     {
-      cancel_backup();
+      cancel_backup();  // Note: never errors.
       return ERROR;
     }
     if (it->state == backup_state::PREPARING)
@@ -946,7 +963,7 @@ int Scheduler::lock()
   for (Pump_iterator it(*this); it; ++it)
    if (it->lock())
    {
-     cancel_backup();
+     cancel_backup();  // Note: never errors.
      return ERROR;
    }
 
@@ -965,7 +982,7 @@ int Scheduler::unlock()
   {
     if (it->unlock())
     {
-      cancel_backup();
+      cancel_backup();  // Note: never errors.
       return ERROR;
     }
     if (it->state == backup_state::FINISHING)
@@ -990,6 +1007,7 @@ Backup_pump::Backup_pump(Snapshot_info &
 {
   DBUG_ASSERT(snap.m_num > 0);
   m_buf.data= NULL;
+  // FIXME: detect errors if reported.
   bitmap_init(&m_closed_streams,
               NULL,
               1 + snap.table_count(),
@@ -1229,6 +1247,8 @@ int Backup_pump::pump(size_t *howmuch)
         if (m_buf.size > 0)
           mode= WRITING;
         else
+          // FIXME: detect errors (perhaps ensure that drop_buf never errors).
+          // FIXME: error logging.
           m_bw.drop_buf(m_buf);
 
         break;
@@ -1247,6 +1267,8 @@ int Backup_pump::pump(size_t *howmuch)
         state= backup_state::DONE;
 
       case BUSY:
+        // FIXME: detect errors (perhaps ensure that drop_buf never errors).
+        // FIXME: error logging.
         m_bw.drop_buf(m_buf);
         m_buf_head=NULL;  // thus a new request will be made
       }
@@ -1502,6 +1524,8 @@ int restore_table_data(THD *thd, Restore
           bad_drivers.append(",");
         bad_drivers.append(info.m_snap[n]->name());
       }
+      // FIXME: detect errors.
+      // FIXME: error logging.
       drv[n]->free();
     }
 
@@ -1520,6 +1544,8 @@ int restore_table_data(THD *thd, Restore
     if (!drv[n])
       continue;
 
+    // FIXME: detect errors.
+    // FIXME: error logging (perhaps we don't want to report errors here).
     drv[n]->free();
   }
 

=== modified file 'sql/backup/image_info.cc'
--- a/sql/backup/image_info.cc	2008-06-18 14:09:34 +0000
+++ b/sql/backup/image_info.cc	2008-07-31 10:45:02 +0000
@@ -8,6 +8,16 @@
 
   @brief Implements @c Image_info class and friends.
 
+  @todo Fix error detection in places marked with "FIXME: detect errors...". 
+  These are places where functions or methods are called and if they can 
+  report errors it should be detected and appropriate action taken. If callee 
+  never reports errors or we want to ignore errors, a comment explaining this
+  should be added.
+
+  @todo Fix error logging in places marked with "FIXME: error logging...". In 
+  these places it should be decided if and how the error should be shown to the
+  user. If an error message should be logged, it can happen either in the place
+  where error was detected or somewhere up the call stack.
 */
 
 namespace backup {
@@ -15,6 +25,7 @@ namespace backup {
 Image_info::Image_info()
   :data_size(0), m_table_count(0), m_dbs(16, 16), m_ts_map(16,16)
 {
+  // FIXME: detect errors if reported.
   init_alloc_root(&mem_root, 4 * 1024, 0);
 
   /* initialize st_bstream_image_header members */
@@ -295,14 +306,15 @@ Image_info::add_table(Db &db, const ::St
   if (!t)
     return NULL;
 
-  if (snap.add_table(*t, pos))
+  if (snap.add_table(*t, pos))  // reports errors
     return NULL;
   
+  // FIXME: error logging.
   if (db.add_table(*t))
     return NULL;
 
   if (!snap.m_num)
-    snap.m_num= add_snapshot(snap);
+    snap.m_num= add_snapshot(snap); // reports errors
 
   if (!snap.m_num)
    return NULL;

=== modified file 'sql/backup/image_info.h'
--- a/sql/backup/image_info.h	2008-07-07 12:51:56 +0000
+++ b/sql/backup/image_info.h	2008-07-31 10:45:02 +0000
@@ -1,6 +1,15 @@
 #ifndef CATALOG_H_
 #define CATALOG_H_
 
+/**
+  @file
+  
+  @todo Fix error logging in places marked with "FIXME: error logging...". In 
+  these places it should be decided if and how the error should be shown to the
+  user. If an error message should be logged, it can happen either in the place
+  where error was detected or somewhere up the call stack.
+*/ 
+
 #include <si_objects.h>
 #include <backup_stream.h> // for st_bstream_* types
 #include <backup/backup_aux.h>  // for Map template
@@ -802,6 +811,7 @@ void Image_info::save_binlog_pos(const :
 inline
 Image_info::Db_iterator* Image_info::get_dbs() const
 {
+  // FIXME: error logging (in case allocation fails).
   return new Db_iterator(*this);
 }
 
@@ -809,6 +819,7 @@ Image_info::Db_iterator* Image_info::get
 inline
 Image_info::Ts_iterator* Image_info::get_tablespaces() const
 {
+  // FIXME: error logging (in case allocation fails).
   return new Ts_iterator(*this);
 }
 
@@ -816,6 +827,7 @@ Image_info::Ts_iterator* Image_info::get
 inline
 Image_info::Dbobj_iterator* Image_info::get_db_objects(const Db &db) const
 {
+  // FIXME: error logging (in case allocation fails).
   return new Dbobj_iterator(*this, db);
 }
 

=== modified file 'sql/backup/kernel.cc'
--- a/sql/backup/kernel.cc	2008-07-09 08:19:03 +0000
+++ b/sql/backup/kernel.cc	2008-07-31 10:45:02 +0000
@@ -50,15 +50,22 @@
   } // if code jumps here, context destructor will do the clean-up automatically
   @endcode
 
+  @todo Fix error detection in places marked with "FIXME: detect errors...". 
+  These are places where functions or methods are called and if they can 
+  report errors it should be detected and appropriate action taken. If callee 
+  never reports errors or we want to ignore errors, a comment explaining this
+  should be added.
+
+  @todo Fix error logging in places marked with "FIXME: error logging...". In 
+  these places it should be decided if and how the error should be shown to the
+  user. If an error message should be logged, it can happen either in the place
+  where error was detected or somewhere up the call stack.
+
   @todo Use internal table name representation when passing tables to
         backup/restore drivers.
   @todo Handle other types of meta-data in Backup_info methods.
   @todo Handle item dependencies when adding new items.
   @todo Handle other kinds of backup locations (far future).
-
-  @note This comment was added to show Jorgen and Oystein how to push patches 
-  into the team tree - please remove it if you see it. Second version of the 
-  patch, to show how to collapse/re-edit csets.
 */
 
 #include "../mysql_priv.h"
@@ -271,17 +278,31 @@ int send_reply(Backup_restore_ctx &conte
   /*
     Send field list.
   */
+  // FIXME: detect errors if  reported.
+  // FIXME: error logging.
   field_list.push_back(new Item_empty_string(STRING_WITH_LEN("backup_id")));
+  // FIXME: detect errors if  reported.
+  // FIXME: error logging.
   protocol->send_fields(&field_list, Protocol::SEND_NUM_ROWS |
Protocol::SEND_EOF);
 
   /*
     Send field data.
   */
+  // FIXME: detect errors if  reported.
+  // FIXME: error logging.
   protocol->prepare_for_resend();
+  // FIXME: detect errors if  reported.
+  // FIXME: error logging.
   llstr(context.op_id(), buf);
+  // FIXME: detect errors if  reported.
+  // FIXME: error logging.
   protocol->store(buf, system_charset_info);
+  // FIXME: detect errors if  reported.
+  // FIXME: error logging.
   protocol->write();
 
+  // FIXME: detect errors if  reported.
+  // FIXME: error logging.
   my_eof(context.thd());
   DBUG_RETURN(0);
 }
@@ -366,8 +387,12 @@ int Backup_restore_ctx::prepare(LEX_STRI
   
   // Prepare error reporting context.
   
+  // FIXME: detect errors if  reported.
+  // FIXME: error logging.
   mysql_reset_errors(m_thd, 0);
   m_thd->no_warnings_for_error= FALSE;
+  // FIXME: detect errors if  reported.
+  // FIXME: error logging.
   save_errors();  
 
 
@@ -669,9 +694,13 @@ int Backup_restore_ctx::lock_tables_for_
       backup::Image_info::Table *tbl= snap->get_table(t);
       DBUG_ASSERT(tbl); // All tables should be present in the catalogue.
 
+      // FIXME: detect errors. Don't assert here but report error instead.
+      // FIXME: error logging.
       TABLE_LIST *ptr= backup::mk_table_list(*tbl, TL_WRITE, m_thd->mem_root);
-      DBUG_ASSERT(ptr);  // FIXME: report error instead
+      DBUG_ASSERT(ptr);
 
+      // FIXME: detect errors if reported.
+      // FIXME: error logging.
       tables= backup::link_table_list(*ptr, tables);      
       tbl->m_table= ptr;
     }
@@ -733,8 +762,10 @@ int Backup_restore_ctx::close()
   time_t when= my_time(0);
 
   // If auto commit is turned off, be sure to commit the transaction
-  // TODO: move it to the big switch, case: MYSQLCOM_BACKUP?
-
+  /* 
+    Note: this code needs to be refactored (see BUG#38261). When refactoring
+    make sure that errors are detected and reported.
+  */
   if (m_thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
   {
     ha_autocommit_or_rollback(m_thd, 0);
@@ -743,10 +774,13 @@ int Backup_restore_ctx::close()
 
   // unlock tables if they are still locked
 
+  // FIXME: detect errors if reported.
   unlock_tables();
 
   // unfreeze meta-data
 
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   obs::ddl_blocker_disable();
 
   // restore thread options
@@ -756,10 +790,12 @@ int Backup_restore_ctx::close()
   // close stream
 
   if (m_stream)
+    // FIXME: detect errors if reported.
+    // FIXME: error logging.
     m_stream->close();
 
   if (m_catalog)
-    m_catalog->save_end_time(when);
+    m_catalog->save_end_time(when); // Note: no errors.
 
   // destroy backup stream's memory allocator (this frees memory)
 
@@ -827,6 +863,8 @@ int Backup_restore_ctx::do_backup()
 
   DEBUG_SYNC(m_thd, "before_backup_meta");
 
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   report_stats_pre(info);
 
   DBUG_PRINT("backup",("Writing preamble"));
@@ -852,6 +890,8 @@ int Backup_restore_ctx::do_backup()
     DBUG_RETURN(m_error);
   }
 
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   report_stats_post(info);
 
   DBUG_PRINT("backup",("Backup done."));
@@ -880,6 +920,7 @@ int Backup_restore_ctx::restore_triggers
 
   DBUG_ASSERT(m_catalog);
 
+  // FIXME: detect errors (when dbit==NULL). Perhaps just assert.
   Image_info::Iterator *dbit= m_catalog->get_dbs();
   Image_info::Obj *obj;
   List<Image_info::Obj> events;
@@ -891,6 +932,7 @@ int Backup_restore_ctx::restore_triggers
   
   while ((obj= (*dbit)++)) 
   {
+    // FIXME: detect errors (when it==NULL). Perhaps just assert.
     Image_info::Iterator *it= 
                    
m_catalog->get_db_objects(*static_cast<Image_info::Db*>(obj));
 
@@ -899,6 +941,8 @@ int Backup_restore_ctx::restore_triggers
       
       case BSTREAM_IT_EVENT:
         DBUG_ASSERT(obj->m_obj_ptr);
+        // FIXME: detect errors if reported.
+        // FIXME: error logging.
         events.push_back(obj);
         break;
       
@@ -961,11 +1005,14 @@ int Backup_restore_ctx::do_restore()
   Input_stream &s= *static_cast<Input_stream*>(m_stream);
   Restore_info &info= *static_cast<Restore_info*>(m_catalog);
 
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   report_stats_pre(info);
 
   DBUG_PRINT("restore", ("Restoring meta-data"));
 
-  disable_fkey_constraints();
+  // FIXME: detect errors if reported.
+  disable_fkey_constraints();  // reports errors
 
   if (read_meta_data(info, s))
   {
@@ -973,6 +1020,7 @@ int Backup_restore_ctx::do_restore()
     DBUG_RETURN(m_error);
   }
 
+  // FIXME: detect errors.
   s.next_chunk();
 
   DBUG_PRINT("restore",("Restoring table data"));
@@ -983,7 +1031,11 @@ int Backup_restore_ctx::do_restore()
     It should be fixed inside object services implementation and then the
     following line should be removed.
    */
+  // FIXME: detect errors.
+  // FIXME: error logging.
   close_thread_tables(m_thd);
+  // FIXME: detect errors.
+  // FIXME: error logging.
   m_thd->main_da.reset_diagnostics_area();
 
   if (lock_tables_for_restore()) // reports errors
@@ -992,6 +1044,7 @@ int Backup_restore_ctx::do_restore()
   // Here restore drivers are created to restore table data
   err= restore_table_data(m_thd, info, s); // reports errors
 
+  // FIXME: detect errors if reported.
   unlock_tables();
 
   if (err)
@@ -1004,7 +1057,7 @@ int Backup_restore_ctx::do_restore()
    creation of these objects will fail.
   */
 
-  if (restore_triggers_and_events())
+  if (restore_triggers_and_events())    // reports errors
      DBUG_RETURN(ER_BACKUP_RESTORE);
 
   DBUG_PRINT("restore",("Done."));
@@ -1021,9 +1074,15 @@ int Backup_restore_ctx::do_restore()
     It should be fixed inside object services implementation and then the
     following line should be removed.
    */
+  // FIXME: detect errors.
+  // FIXME: error logging.
   close_thread_tables(m_thd);
+  // FIXME: detect errors.
+  // FIXME: error logging.
   m_thd->main_da.reset_diagnostics_area();
 
+  // FIXME: detect errors if reported.
+  // FIXME: error logging.
   report_stats_post(info);
 
   DBUG_RETURN(0);