Bug #37918 Online backup: Incorrect table locking during RESTORE
Submitted: 7 Jul 2008 8:43 Modified: 21 Aug 2008 18:22
Reporter: Rafal Somla Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: Backup Severity:S3 (Non-critical)
Version:6.0-backup OS:Any
Assigned to: Rafal Somla CPU Architecture:Any

[7 Jul 2008 8:43] Rafal Somla
Description:
During RESTORE operation backup kernel needs to:

a) block access to all tables being restored (native drivers depend on that)
b) open tables for the built-in, logical drivers.

Currently this is done as follows:

a) tables restored by native drivers are name locked using lock_table_names_exclusively(),

b) tables processed by the built-in drivers are opened and locked using open_and_lock_tables().

But this is not correct to name_lock same tables while open_and_lock others (in a single thread), as runtime team says. It works in the current mysql-6.0-backup which is based on an old main tree but it will break after updating the tree, because runtime team adds asserts to guard against such illegal locking strategies.

How to repeat:
Code inspection.

Suggested fix:
Use open_and_lock_tables() for all tables being restored. Don't use name locks.
[7 Jul 2008 8:58] Rafal Somla
PROPOSED SOLUTION
-----------------

All tables being restored will be opened and locked in TL_WRITE mode in the backup kernel. The built-in drivers will get access to the tables opened by the kernel.

The opening and locking of tables will be performed in Backup_restore_ctx::do_restsore() using new helper methods:

  int lock_tables_for_restore();
  int unlock_tables();

Lock_tables_for_restore() will use simple_open_n_lock_tables() to open and lock the tables. This function needs a linked TABLE_LIST as input - the list will be constructed inside lock_tables_for_restore() based on the contents of restore catalogue.

For each table, a pointer to the corresponding TABLE_LIST structure will be stored in the catalogue entry for that table, so that one can get access to the opened TABLE structure after the tables have been opened. For that, a new member

 TABLE_LIST *m_table;

will be added to Image_info::Table class. This pointer will be set inside lock_tables_for_restore() when the list of tables for locking is created.

To give built-in drivers access to the opened tables, the following changes will be introduced:

1. A new class Logical_snapshot will be defined. This class will extend Snapshot_info with a method

 TABLE *get_opened_table(ulong pos);

returning pointer to an opened TABLE structure for table at given position in the snapshot. This method will use the m_table pointer in the catalogue entry corresponding to the given table.

2. The built-in restore drivers (default_backup::Restore and snapshot_backup::Restore) will store a reference to their snapshot object (which will be of type Logical_snapshot) and use its get_opened_table() method to access the opened TABLE structures.
[7 Jul 2008 8:59] Rafal Somla
Set to "Critical" since we need this to be fixed before we can merge with the main tree.
[7 Jul 2008 9:50] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/49070

2649 Rafal Somla	2008-07-07
      BUG#37918 (Online backup: Incorrect table locking during RESTORE)
      
      Current code uses name locks for locking tables during restore and at the same 
      time opens and locks tables for the built-in drivers. It is not correct to 
      name-lock some tables and open some other tables at the same time.
      
      This patch fixes it by not using name locks, but openning and locking all the tables being restored.
[7 Jul 2008 12:45] Chuck Bell
Patch approved with addition/clarification of a comment on unlock_tables().
[7 Jul 2008 12:53] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/49091

2649 Rafal Somla	2008-07-07
      BUG#37918 (Online backup: Incorrect table locking during RESTORE)
           
      Current code uses name locks for locking tables during restore and at the same 
      time opens and locks tables for the built-in drivers. It is not correct to 
      name-lock some tables and open some other tables at the same time.
            
      This patch fixes it by not using name locks, but openning and locking all the
[7 Jul 2008 14:00] Ingo Strüwing
Ok to push after thinking about some comments, sent by email.
[8 Jul 2008 19:59] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/49242

2650 Rafal Somla	2008-07-08
      BUG#37918: post review changes.
[9 Jul 2008 8:37] Rafal Somla
Pushed into mysql-6.0-backup.
[20 Aug 2008 13:32] Chuck Bell
Pushed to 6.0.0.
[21 Aug 2008 18:22] Paul DuBois
Pushed to 6.0.5, the same release when BACKUP DATABASE/RESTORE originally appeared. So this bug is not in any released version; no changelog entry needed.
[13 Sep 2008 21:51] Bugs System
Pushed into 6.0.6-alpha  (revid:rsomla@mysql.com-20080707125156-e2w0hz86p933xggq) (version source revid:hakan@mysql.com-20080716105246-eg0utbybp122n2w9) (pib:3)
[21 Dec 2009 19:26] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/95303

3021 Chuck Bell	2009-12-21
      BUG#37918 (Online backup: Incorrect table locking during RESTORE)
      
      Current code uses name locks for locking tables during restore and at the same 
      time opens and locks tables for the built-in drivers. It is not correct to 
      name-lock some tables and open some other tables at the same time.
      
      This patch fixes it by not using name locks, but openning and locking all the 
      
      original changeset: 2599.21.1
     @ sql/backup/be_logical.h
        Definition of Logical_snapshot class.