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

[Øystein Grøvlen]

Approved. OK to push.

--
Øystein

Rafal Somla wrote:
> #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);
> 
>