Re: bzr commit into mysql-6.0-backup-myisam branch (cbell:2618) Bug#36749, Bug#36778, Bug#36782

[Rafal Somla]

Hi Chuck,

Thanks for taking into account my comments so far. I still have one final 
remark. You made the internal type Image_info::Tables public (probably I used 
this type in my earlier suggestions). But actually doing this is not necessary. 
Class Image_info::Tables implements public interface backup::Table_list and it 
is enough to use the public interface (see below).

After fixing this the patch is good to push.

Chuck Bell wrote:
> #At file:///D:/source/bzr/mysql-6.0-bug-36749/
> 
>  2618 Chuck Bell	2008-06-20
>       BUG#36749 Data Loss after Restore, if Trigger fired on the table that is being
> Restored. 
>       
>       Backup kernel should lock all tables during restore but it was not doing so
>       for native drivers. This caused the MyISAM storage engine to return incorrect 
>       and loss of data when tables were being restored.
>       
>       Note: This patch also fixes BUG#36778 and BUG#36782.
> added:
>   mysql-test/r/backup_lock_myisam.result
>   mysql-test/t/backup_lock_myisam.test
> modified:
>   sql/backup/data_backup.cc
>   sql/backup/image_info.h
>   sql/share/errmsg.txt
>   sql/si_objects.cc
>   sql/si_objects.h
> 
> per-file messages:
>   mysql-test/r/backup_lock_myisam.result
>     New result file.
>   mysql-test/t/backup_lock_myisam.test
>     New test for MyISAM locking problem.
>   sql/backup/data_backup.cc
>     Added code to obtain name locks on all tables prior to restore and release them
> after
>     restore. Code includes method to gather tables from all snapshots.
>   sql/backup/image_info.h
>     Needed to expose Tables class for retrieving list of tables for lock.
>   sql/share/errmsg.txt
>     New error messages added.
>   sql/si_objects.cc
>     Added new class to manage name lock on a table list. Class was needed
>     to preserve state between get_ and release_ of locks.
>   sql/si_objects.h
>     Added primitives for new name locking class.
(cut)
> === modified file 'sql/backup/data_backup.cc'
> --- a/sql/backup/data_backup.cc	2008-05-05 15:03:24 +0000
> +++ b/sql/backup/data_backup.cc	2008-06-20 13:59:11 +0000
> @@ -1364,7 +1364,7 @@ namespace backup {
>  /**
>    Read backup image data from a backup stream and forward it to restore drivers.
>   */
> -int restore_table_data(THD*, Restore_info &info, Input_stream &s)
> +int restore_table_data(THD *thd, Restore_info &info, Input_stream &s)
>  {
>    DBUG_ENTER("restore::restore_table_data");
>  
> @@ -1377,6 +1377,8 @@ int restore_table_data(THD*, Restore_inf
>  
>    TABLE_LIST *table_list= 0;
>    TABLE_LIST *table_list_last= 0;
> +  List<obs::Obj> tables_to_lock;
> +  obs::Name_locker *table_name_locker= new obs::Name_locker(thd);
>  
>    if (info.snap_count() > 256)
>    {
> @@ -1412,6 +1414,15 @@ int restore_table_data(THD*, Restore_inf
>          (snap->type() == Snapshot_info::CS_SNAPSHOT))
>        get_default_snapshot_tables(NULL, (default_backup::Restore *)drv[n],
>                                    &table_list, &table_list_last);
> +    /*
> +      Collect tables from all drivers for name locking.
> +    */
> +    Image_info::Tables *snap_table_list= snap->get_table_list();
> +    for (uint i= 0; i < snap_table_list->count(); i++)
> +    {
> +      Image_info::Table *tbl= snap_table_list->get_table(i);

Instead of using private type Image_info::Tables, one can use the public 
interface backup::Table_list. In that case snap_table_list->get_table(i) would 
be replaced by (*snap_table_list)[i].

> +      tables_to_lock.push_front(tbl->m_obj_ptr);
> +    }
>    }
>  
>    /*
> @@ -1430,6 +1441,15 @@ int restore_table_data(THD*, Restore_inf
>        table_list_last->next_global= NULL; // break lists
>    }
>  
> +  /*
> +    Apply name locks to all tables used.
> +  */
> +  if (table_name_locker->get_name_locks(&tables_to_lock, TL_WRITE))
> +  {
> +    info.m_ctx.fatal_error(ER_BACKUP_OBTAIN_NAME_LOCK_FAILED);
> +    goto error;
> +  }
> +
>    // Initialize the drivers.
>    for (uint n=0; n < info.snap_count(); ++n)
>    {
> @@ -1441,6 +1461,7 @@ int restore_table_data(THD*, Restore_inf
>      }
>    }
>  
> +  DEBUG_SYNC(thd, "restore_in_progress");
>    {
>      Buffer  buf;
>      uint    snap_num=0;
> @@ -1592,6 +1613,13 @@ int restore_table_data(THD*, Restore_inf
>    }
>  
>    /*
> +    Release name locks on driver tables.
> +  */
> +  if (table_name_locker->release_name_locks())
> +    info.m_ctx.fatal_error(ER_BACKUP_RELEASE_NAME_LOCK_FAILED);
> +  delete table_name_locker;
> +
> +  /*
>      Close all tables if default or snapshot driver used.
>    */
>    if (table_list)
> @@ -1602,6 +1630,9 @@ int restore_table_data(THD*, Restore_inf
>   error:
>  
>    DBUG_PRINT("restore",("Cancelling restore process"));
> +
> +  if (table_name_locker)
> +    delete table_name_locker;
>  
>    for (uint n=0; n < info.snap_count(); ++n)
>    {
> 
> === modified file 'sql/backup/image_info.h'
> --- a/sql/backup/image_info.h	2008-05-05 15:06:40 +0000
> +++ b/sql/backup/image_info.h	2008-06-20 13:59:11 +0000
> @@ -132,9 +132,10 @@ public: // public interface
>    Image_info();
>    uint m_table_count;
>  
> +  class Tables; ///< Implementation of Table_list interface. 
> +
>   private:
>  
> -  class Tables; ///< Implementation of Table_list interface. 
>

This change is not needed - see below.

>    // storage
>  
> @@ -260,6 +261,8 @@ class Snapshot_info
>    virtual result_t get_restore_driver(Restore_driver*&) =0;
>  
>    virtual ~Snapshot_info();
> +
> +  Image_info::Tables *get_table_list() { return &m_tables; }

Instead of making Image_info::Tables public, you should declare get_table_list() 
to return backup::Table_list* type. The internal type Image_info::Tables 
implements the public interface backup::Table_list. Clients of this method do 
not need to know about the implementation - all they care about is the interface.

>  
>   protected:
>   
> 
> === modified file 'sql/share/errmsg.txt'
> --- a/sql/share/errmsg.txt	2008-05-05 19:38:05 +0000
> +++ b/sql/share/errmsg.txt	2008-06-20 13:59:11 +0000
> @@ -6357,3 +6357,8 @@ ER_DEBUG_SYNC_HIT_LIMIT
>    eng "debug sync point hit limit reached"
>    ger "Debug Sync Point Hit Limit erreicht"
>  
> +ER_BACKUP_OBTAIN_NAME_LOCK_FAILED
> +  eng "Restore failed to obtain the name locks on the tables."
> +ER_BACKUP_RELEASE_NAME_LOCK_FAILED
> +  eng "Restore failed to release the name locks on the tables."
> +
> 
> === modified file 'sql/si_objects.cc'
> --- a/sql/si_objects.cc	2008-05-05 20:19:03 +0000
> +++ b/sql/si_objects.cc	2008-06-20 13:59:11 +0000
> @@ -3071,6 +3071,96 @@ void ddl_blocker_exception_off(THD *thd)
>    DBUG_VOID_RETURN;
>  }
>  
> +/**
> +  Build a table list from a list of tables as class Obj.
> +
> +  This method creates a TABLE_LIST from a List<> of type Obj.
> +
> +  param[IN]  tables    The list of tables
> +  param[IN]  lock      The desired lock type
> +
> +  @returns TABLE_LIST *
> +
> +  @note Caller must free memory.
> +*/
> +TABLE_LIST *Name_locker::build_table_list(List<Obj> *tables,
> +                                          thr_lock_type lock)
> +{
> +  TABLE_LIST *tl= NULL;
> +  Obj *tbl= NULL;
> +  DBUG_ENTER("Name_locker::build_table_list()");
> +  
> +  List_iterator<Obj> it(*tables);
> +  while (tbl= it++)
> +  {
> +    TABLE_LIST *ptr= (TABLE_LIST*)my_malloc(sizeof(TABLE_LIST), MYF(MY_WME));
> +    DBUG_ASSERT(ptr);  // FIXME: report error instead
> +    bzero(ptr, sizeof(TABLE_LIST));
> +
> +    ptr->alias= ptr->table_name=
> const_cast<char*>(tbl->get_name()->ptr());
> +    ptr->db= const_cast<char*>(tbl->get_db_name()->ptr());
> +    ptr->lock_type= lock;
> +
> +    // and add it to the list
> +
> +    ptr->next_global= ptr->next_local=
> +      ptr->next_name_resolution_table= tl;
> +    tl= ptr;
> +    tl->table= ptr->table;
> +  }
> +
> +  DBUG_RETURN(tl);
> +}
> +
> +/**
> +  Gets name locks on table list.
> +
> +  This method attempts to take an exclusive name lock on each table in the
> +  list. It does nothing if the table list is empty.
> +
> +  @param[IN] tables  The list of tables to lock.
> +  @param[IN] lock    The type of lock to take.
> +
> +  @returns 0 if success, 1 if error
> +*/
> +int Name_locker::get_name_locks(List<Obj> *tables, thr_lock_type lock)
> +{
> +  int ret= 0;
> +  DBUG_ENTER("Name_locker::get_name_locks()");
> +  /*
> +    Convert List<Obj> to TABLE_LIST *
> +  */
> +  m_table_list= build_table_list(tables, lock);
> +  if (m_table_list)
> +  {
> +    pthread_mutex_lock(&LOCK_open);
> +    if (lock_table_names_exclusively(m_thd, m_table_list))
> +      ret= 1;
> +    pthread_mutex_unlock(&LOCK_open);
> +  }
> +  DBUG_RETURN(ret);
> +}
> +
> +/*
> +  Releases name locks on table list.
> +
> +  This method releases the name locks on the table list. It does nothing if
> +  the table list is empty.
> +
> +  @returns 0 if success, 1 if error
> +*/
> +int Name_locker::release_name_locks()
> +{
> +  DBUG_ENTER("Name_locker::release_name_locks()");
> +  if (m_table_list)
> +  {
> +    pthread_mutex_lock(&LOCK_open);
> +    unlock_table_names(m_thd, m_table_list, (TABLE_LIST*) 0);
> +    pthread_mutex_unlock(&LOCK_open);
> + }
> +  DBUG_RETURN(0);
> +}
> +
>  } // obs namespace
>  
>  ///////////////////////////////////////////////////////////////////////////
> 
> === modified file 'sql/si_objects.h'
> --- a/sql/si_objects.h	2008-05-05 15:03:24 +0000
> +++ b/sql/si_objects.h	2008-06-20 13:59:11 +0000
> @@ -604,6 +604,38 @@ COND *create_db_select_condition(THD *th
>                                   TABLE *t,
>                                   List<LEX_STRING> *db_list);
>  
> +/*
> +  The following class is used to manage name locks on a list of tables.
> +
> +  This class uses a list of type List<Obj> to establish the table list 
> +  that will be used to manage locks on the tables.
> +*/
> +class Name_locker
> +{
> +public:
> +  Name_locker(THD *thd) { m_thd= thd; }
> +  ~Name_locker() { my_free(m_table_list, MYF(0)); }
> +
> +  /*
> +    Gets name locks on table list.
> +  */
> +  int get_name_locks(List<Obj> *tables, thr_lock_type lock);
> +
> +  /*
> +    Releases name locks on table list.
> +  */
> +  int release_name_locks();
> +
> +private:
> +  TABLE_LIST *m_table_list; ///< The list of tables to obtain locks on.
> +  THD *m_thd;               ///< Thread context.
> +
> +  /*
> +    Builds a table list from the list of objects passed to constructor.
> +  */
> +  TABLE_LIST *build_table_list(List<Obj> *tables, thr_lock_type lock);
> +};
> +

Having such a class, it is a pity you don't unlock tables in the destructor. 
When this is done, one can use code like this:

{
   Name_locker locks(thd);
   locks.get_name_locks(tables,TL_READ);

   ...

   // Now we don't have to remember about calling release_name_locks() because
   // locks destructor will do that for us when we leave the scope.
}

This is just how I would do that - it is ok to leave the code as it is.

Rafal