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

[Rafal Somla]

Hi Chuck,

Please consider my comments below.

Alik: could you read the comments at the end about data types used in si_objects 
extension proposed by Chuck.

Chuck Bell wrote:
> #At file:///C:/source/bzr/mysql-6.0-bug-36749/
> 
>  2618 Chuck Bell	2008-06-17
>       BUG#36749 Data Loss after Restore, if Trigger fired on the table that is being
> Restored.
>       
>       The MyISAM native driver is not locking its tables. This results in data losson
> restore
>       as either a collision or an overwrite condition occurs.
>       
>       This patch corrects the problem by taking an exclusive name lock on all tables
> prior to
>       restore of the data (after the metadata step).

I would describe the problem differently: Backup kernel should lock all tables 
during restore but it was not doing so. This caused MyISAM native driver to fail 
when another client was accessing tables being restored.

> 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
>     Added method to return list of tables from snapshot.
>   sql/share/errmsg.txt
>     New error message for name locking errors.
>   sql/si_objects.cc
>     Added code to obtain and release name locks.
>   sql/si_objects.h
>     New method primitives for obtaining and releasing name locks.

> === 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-17 12:40:00 +0000
> @@ -342,6 +342,35 @@ int get_default_snapshot_tables(backup::
>    DBUG_RETURN(0);
>  }
>  
> +/*
> +  Collect tables from all drivers for taking name lock on the tables.
> +*/
> +int get_all_snapshot_tables(Table_list *t,
> +                            TABLE_LIST **tables,
> +                            TABLE_LIST **tables_last)
> +{
> +  TABLE_LIST *table_list= *tables;
> +  TABLE_LIST *table_list_last= *tables_last;
> +
> +  DBUG_ENTER("backup::get_all_snapshot_tables");
> +  /*
> +    If the table list is empty, use the first one and loop
> +    until the end then record the end of the first one.
> +  */
> +  if (!table_list)
> +  {
> +    table_list= build_table_list(*t, TL_WRITE);
> +    *tables= table_list;
> +    table_list_last= table_list;
> +    while (table_list_last->next_global != NULL)
> +      table_list_last= table_list_last->next_global;
> +    *tables_last= table_list_last;
> +  }
> +  else
> +    (*tables_last)->next_global= build_table_list(*t, TL_WRITE);

Is there a stronger lock, like TL_WRITE_NO_SELECT?

> +  DBUG_RETURN(0);
> +}
> +
>  /**
>     Commit Blocker
>  
> @@ -1364,7 +1393,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 +1406,8 @@ int restore_table_data(THD*, Restore_inf
>  
>    TABLE_LIST *table_list= 0;
>    TABLE_LIST *table_list_last= 0;
> +  TABLE_LIST *all_tables= 0;
> +  TABLE_LIST *all_tables_last= 0;
>  
>    if (info.snap_count() > 256)
>    {
> @@ -1412,6 +1443,11 @@ 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.
> +    */
> +    get_all_snapshot_tables((Table_list *)snap->get_table_list(),
> +                            &all_tables, &all_tables_last);

I don't see why you need to define a separate function for appending tables to 
the global list. Note that you use this function only here in the code and it is 
unlikely that it could be used elsewhere.

The same effect could be achieved for example with this code:

   TABLE_LIST *ptr= build_table_list(snap->get_table_list(),TL_WRITE);

   while (ptr)
   {
      TABLE_LIST *next= ptr->next_global;
      ptr->next_global= all_tables;
      all_tables= ptr;
      ptr= next;
   }

>    }
>  
>    /*
> @@ -1430,6 +1466,15 @@ int restore_table_data(THD*, Restore_inf
>        table_list_last->next_global= NULL; // break lists
>    }
>  
> +  /*
> +    Apply name locks to all tables used.
> +  */
> +  if (obs::get_name_locks(thd, all_tables, TRUE))
> +  {
> +    info.m_ctx.fatal_error(ER_BACKUP_RESTORE_NAME_LOCK_FAILED, "obtain");

Inserting English words like that into error message will create problems with 
internationalization. I think we should use separate message for each case.

> +    goto error;
> +  }
> +
>    // Initialize the drivers.
>    for (uint n=0; n < info.snap_count(); ++n)
>    {
> @@ -1441,6 +1486,7 @@ int restore_table_data(THD*, Restore_inf
>      }
>    }
>  
> +  DEBUG_SYNC(thd, "restore_in_progress");
>    {
>      Buffer  buf;
>      uint    snap_num=0;
> @@ -1590,6 +1636,12 @@ int restore_table_data(THD*, Restore_inf
>      if (!bad_drivers.is_empty())
>        info.m_ctx.report_error(ER_BACKUP_STOP_RESTORE_DRIVERS, bad_drivers.c_ptr());
>    }
> +
> +  /*
> +    Release name locks on driver tables.
> +  */
> +  if (obs::release_name_locks(thd, all_tables))
> +    info.m_ctx.fatal_error(ER_BACKUP_RESTORE_NAME_LOCK_FAILED, "release");
>  
>    /*
>      Close all tables if default or snapshot driver used.
> 
> === 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-17 12:40:00 +0000
> @@ -261,6 +261,8 @@ class Snapshot_info
>  
>    virtual ~Snapshot_info();
>  
> +  Image_info::Tables *get_table_list() { return &m_tables; }
> +
>   protected:
>   
>    version_t m_version; ///< Stores version number of the snapshot's format.
> 
> === 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-17 12:40:00 +0000
> @@ -6357,3 +6357,6 @@ ER_DEBUG_SYNC_HIT_LIMIT
>    eng "debug sync point hit limit reached"
>    ger "Debug Sync Point Hit Limit erreicht"
>  
> +ER_BACKUP_RESTORE_NAME_LOCK_FAILED
> +  eng "Restore failed to %-64s 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-17 12:40:00 +0000
> @@ -3071,6 +3071,64 @@ void ddl_blocker_exception_off(THD *thd)
>    DBUG_VOID_RETURN;
>  }
>  
> +/**
> +  Gets name locks on table list.
> +
> +  This method attempts to take a name lock on each table in the list.
> +  The lock can be exclusive or not. It does nothing if the table list is
> +  empty.
> +
> +  @param[IN] thd        current thread
> +  @param[IN] tables     List of tables to take name locks
> +  @param[IN] exclusive  If true, the operation takes and exclusive name lock
> +
> +  @returns 0 if success, 1 if error
> +*/
> +int get_name_locks(THD *thd, TABLE_LIST *tables, bool exclusive)

In all other si_object functions we identify databases and tables using strings 
containing their names. E.g. "Obj *get_table(const String *db_name, const String 
*table_name)". This is important because we want to be able to use the API from 
an independent module which should ideally compile without a need to include all 
the server code. And TABLE_LIST type is very much interconnected with all the 
rest of the server code, so it is not a good candidate.

I think the most elegant solution would be to pass a list of table objects 
obtained earlier from get_table() or other SI function. That is a list of 
obs::Obj instances corresponding to the tables which should be locked. What type 
to use for that list I'm not sure. One option would be to use List<obs::Obj> but 
then we again create dependency on the server code (however, we already use 
String type). Other option would be to define an ObjList type in si_objects.

 From the backup code perspective: would it be difficult to obtain such list of 
obs::Obj instances for the call to get_name_locks()? It is not so bad. The 
tables we want to lock are stored in Snapshot_info::m_tables member which is of 
type Image_info::Tables. The latter stores list of Image_info::Table object. 
Each Table object has a pointer to the corresponding obs::Obj instance stored in 
m_obj_ptr member. So the code for collecting list of tables for locking could 
look as follows:

   List<obs::Obj> tables_to_lock;

   ....

   for (each snapshot object snap)
   {
     ...
     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);
        tables_to_lock.push_back(tbl->m_obj_ptr);
     }
   }

Rafal