Re: bk commit into 5.2 tree (cbell:1.2607) BUG#31383

[Rafal Somla]

Hi Chuck,

The problem with this change is that the image created with default, blocking 
driver will potentially be inconsistent with other images:

..
dd->lock()
od->lock()
<-- VP is here
dd->unlock()
od->unlock()

<open and lock tables for default driver>

dd->get_data()
..

If dd is the default driver, it will give us data from the time when tables were 
locked and this might be different from the data at the VP time...

Given the issues you found with starting CS transaction, I think we have no 
option but to use separate threads for table locking. We can borrow (steal ;)) 
the code which Guilhem wrote for his MyISAM backup engine.

If we lock tables in a separate thread, then perhaps it makes more sense to 
leave this task to backup drivers. That is (contrary to what I argued for 
before), backup kernel will not lock any tables and if a driver needs tables 
locked, it will have to handle this itself, as MyISAM driver does. What do you 
think?

Rafal

cbell@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.2 repository of cbell. When cbell does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2007-10-05 10:53:15-04:00, cbell@mysql_cab_desk. +2 -0
>   BUG#31383 : Consistent Snapshot fails to initiate consistent read
>   
>   This patch modifies the kernel to move the open and lock tables call until after
> the calls to
>   lock and unlock so that the consistent snapshot driver can initiate its consistent
> read (which
>   occurs in lock).
> 
>   sql/backup/be_default.cc@stripped, 2007-10-05 10:53:11-04:00, cbell@mysql_cab_desk. +1
> -0
>     Added debug macro to be used to lock the backup driver while reading. This can be
> used to
>     demonstrate the consistent read behavior of the driver.
> 
>   sql/backup/data_backup.cc@stripped, 2007-10-05 10:53:12-04:00, cbell@mysql_cab_desk.
> +14 -14
>     Moved the open_and_lock_tables() call for the default and consistent snapshot
> drivers to after
>     the calls for lock/unlock.
> 
> diff -Nrup a/sql/backup/be_default.cc b/sql/backup/be_default.cc
> --- a/sql/backup/be_default.cc	2007-07-02 13:42:58 -04:00
> +++ b/sql/backup/be_default.cc	2007-10-05 10:53:11 -04:00
> @@ -293,6 +293,7 @@ result_t Backup::get_data(Buffer &buf)
>      cur_blob= 0;
>      cur_table->use_all_columns();
>      last_read_res = hdl->rnd_next(cur_table->record[0]);
> +    BACKUP_SYNC("backup_snapshot");
>      DBUG_EXECUTE_IF("SLEEP_DRIVER", sleep(4););
>      /*
>        If we are end of file, stop the read process and signal the
> diff -Nrup a/sql/backup/data_backup.cc b/sql/backup/data_backup.cc
> --- a/sql/backup/data_backup.cc	2007-09-11 05:29:29 -04:00
> +++ b/sql/backup/data_backup.cc	2007-10-05 10:53:12 -04:00
> @@ -508,6 +508,20 @@ int write_table_data(THD*, Backup_info &
>      if (sch.prepare())
>        goto error;
>  
> +    while (sch.prepare_count > 0)
> +    if (sch.step())
> +      goto error;
> +
> +    // VP creation
> +    DBUG_PRINT("backup/data",("-- SYNC PHASE --"));
> +    BACKUP_SYNC("data_lock");
> +    if (sch.lock())
> +      goto error;
> +
> +    BACKUP_SYNC("data_unlock");
> +    if (sch.unlock())
> +      goto error;
> +
>      /*
>        Open tables for default and snapshot drivers.
>      */
> @@ -523,20 +537,6 @@ int write_table_data(THD*, Backup_info &
>        if (table_list_last)
>          table_list_last->next_global= NULL; // break lists
>      }
> -
> -    while (sch.prepare_count > 0)
> -    if (sch.step())
> -      goto error;
> -
> -    // VP creation
> -    DBUG_PRINT("backup/data",("-- SYNC PHASE --"));
> -    BACKUP_SYNC("data_lock");
> -    if (sch.lock())
> -      goto error;
> -
> -    BACKUP_SYNC("data_unlock");
> -    if (sch.unlock())
> -      goto error;
>  
>      // get final data from drivers
>      DBUG_PRINT("backup/data",("-- FINISH PHASE --"));
> 
>