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

[Rafal Somla]

Hi Chuck!

The patch looks OK. I have some comments and suggestions concerning structure of 
the code. I think the structure remains from the times where both CS and default 
drivers used separate threads but now it is only the default driver so it looks 
bit strange to split the job into several functions. I wrote my comments about 
that inline.

When you test CS driver using "backup_snapshot" breakpoint, the driver stops 
just after opening the tables in the first get_data() after lock(). But it would 
be interesting to see what happens if it stops after starting CS read 
transaction (in lock()) but before it opens the tables. Also in that case the 
new rows should not appear in the backup image.

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
> 

> 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-19 18:00:14 -04:00
> @@ -126,6 +126,55 @@ Backup::Backup(const Table_list &tables,
>  }
>  
>  /**
> +   @brief Lock tables in driver.
> +
> +   This method creates a new THD for use in the new thread. It calls
> +   the method to open and lock the tables.
> +  */

Looks like this method performs two tasks which are very loosely related. If 
this method is preserved, I'd change it to implement the following behaviour:

/**
   Lock tables in driver.

   This method opens and locks (in TL_WRITE_NO_INSERT mode) all tables in the @c
   tables_in_backup list. A separate thread @c lock_thd is created to do the job,
   so that this operation doesn't interfere with other tables locked by the
   calling thread.
*/

> +void Backup::lock_driver_tables()
> +{
> +  DBUG_ENTER("Default_backup::lock_tables");
> +  lock_thd= create_new_thd();
> +  lock_thread_tables(&lock_thd, &lock_state, tables_in_backup);
> +  lock_called= TRUE;
> +  DBUG_VOID_RETURN;
> +}
> +
> +/**
> +  * @brief Entry point for the locking thread.
> +  *
> +  * The method calls the class method lock_driver_tables which
> +  * handles the open and locking of tables for the driver.
> +  */
> +pthread_handler_t default_thread_for_locking(void *arg)
> +{
> +  my_thread_init();
> +  DBUG_ENTER("Default_backup - lock_tables_in_separate_thread");
> +  (static_cast<Backup *>(arg))->lock_driver_tables();

Since lock_driver_tables() method is not used anywhere else and is very short I 
suggest removing it and calling lock_thread_tables() directly here. The lock_thd
can be created inside prelock().

> +  my_thread_end();
> +  pthread_exit(0);
> +  DBUG_RETURN(0);
> +}
> +
> +/**
> +  * @brief Prelock call to setup locking.
> +  *
> +  * Launches a separate thread ("locking thread") which will lock
> +  * tables. Locking in a separate thread is needed to have a non-blocking
> +  * prelock() (given that thr_lock() is blocking).
> +  */
> +result_t Backup::prelock()
> +{
> +  DBUG_ENTER("Default_backup::open_tables");
> +  lock_state= LOCK_IN_PROGRESS;
> +  if (pthread_create(&th, &connection_attrib,
> +                     default_thread_for_locking, this))
> +    SET_STATE_TO_ERROR_AND_DBUG_RETURN;
> +  pthread_join(th, 0);
> +  DBUG_RETURN(backup::OK);
> +}
> +

This is how prelock() might look after removing the lock_driver_tables() method.

result_t Backup::prelock()
{
   DBUG_ENTER("Default_backup::open_tables");
   lock_state= LOCK_IN_PROGRESS;
   lock_thd= create_new_thd();
   if (pthread_create(&th, &connection_attrib,
                      default_thread_for_locking, this))
     SET_STATE_TO_ERROR_AND_DBUG_RETURN;
   pthread_join(th, 0);
   lock_called= TRUE;
   DBUG_RETURN(backup::OK);
}

Alternatively (if you think that this functionality might be useful in other 
contexts) all this code could be put into the lock_driver_tables() method and 
inside prelock() there would be a single call to it.

result_t Backup::prelock()
{
   return lock_driver_tables();
}

The point is that things which need to be done to lock the tables by spawning 
the locking thread are not spread across several functions/methods without a reason.

> +/**
>    * @brief Start table read.
>    *
>    * This method saves the handler for the table and initializes the
> @@ -198,6 +247,22 @@ int Backup::next_table()
>      }
>    }
>    DBUG_RETURN(0);
> +}
> +
> +/**
> + * @brief End backup process.
> + *
> + * This method unlocks all of the tables.
> + *
> + * @retval backup::OK    all tables unlocked.
> + */
> +result_t Backup::end()
> +{
> +  DBUG_ENTER("Default_backup::end");
> +  close_thread_tables(lock_thd);
> +  net_end(&lock_thd->net);
> +  delete lock_thd;
> +  DBUG_RETURN(OK);
>  }
>  
>  /**

> diff -Nrup a/sql/backup/be_snapshot.cc b/sql/backup/be_snapshot.cc
> --- a/sql/backup/be_snapshot.cc	2007-07-02 13:42:59 -04:00
> +++ b/sql/backup/be_snapshot.cc	2007-10-19 18:00:16 -04:00
> @@ -88,6 +88,7 @@ result_t Backup::end()
>  {
>    DBUG_ENTER("Snapshot_backup::end");
>    end_active_trans(m_thd);
> +  close_thread_tables(m_thd);
>    DBUG_RETURN(OK);
>  }
>  
> @@ -112,6 +113,18 @@ result_t Backup::unlock()
>  {
>    DBUG_ENTER("Snapshot_backup::unlock()");
>    DBUG_RETURN(OK);
> +}
> +
> +result_t Backup::get_data(Buffer &buf)
> +{
> +  if (!tables_open && lock_called)
> +  {
> +    open_and_lock_tables(m_thd, tables_in_backup);
> +    tables_open= TRUE;
> +  }
> +  if (lock_called)
> +    BACKUP_SYNC("backup_snapshot");

Would be good to stop also before opening the tables but after start_trans()...

> +  return(default_backup::Backup::get_data(buf));
>  }
>  
>  /**


> diff -Nrup a/sql/backup/be_thread.cc b/sql/backup/be_thread.cc
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/sql/backup/be_thread.cc	2007-10-19 18:00:19 -04:00
> @@ -0,0 +1,125 @@
> +/* Copyright (C) 2004-2007 MySQL AB
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
> +*/
> +
> +/**
> +  * @file
> +  *
> +  * @brief Contains the thread methods for online backup.
> +  *
> +  * The methods in this class are used to initialize the mutexes
> +  * for the backup threads. Helper methods are included to make thread
> +  * calls easier for the driver code.
> +  */
> +
> +#include "be_thread.h"
> +
> +/**
> +  *  @brief Creates a new THD object.
> +  *
> +  * Creates a new THD object for use in running the open and lock
> +  * method as a separate thread.
> +  *
> +  * @returns Pointer to new THD object or 0 if error.
> +  */
> +THD *create_new_thd()
> +{
> +  THD *thd;
> +  DBUG_ENTER("Online backup::Create new THD object");
> +
> +  thd= new THD;
> +  if (unlikely(!thd))
> +  {
> +    delete thd;
> +    DBUG_RETURN(0);
> +  }
> +
> +  thd->thread_stack = (char*)&thd; // remember where our stack is
> +  pthread_mutex_lock(&LOCK_thread_count);
> +  thd->thread_id= thread_id++;
> +  pthread_mutex_unlock(&LOCK_thread_count);
> +  if (unlikely(thd->store_globals())) // for a proper MEM_ROOT
> +  {
> +    delete thd;
> +    DBUG_RETURN(0);
> +  }
> +
> +  thd->init_for_queries(); // opening tables needs a proper LEX
> +  thd->command= COM_DAEMON;
> +  thd->system_thread= SYSTEM_THREAD_BACKUP;
> +  thd->version= refresh_version;
> +  thd->set_time();
> +  thd->main_security_ctx.host_or_ip= "";
> +  thd->client_capabilities= 0;
> +  thd->mysys_var= 0;
> +  my_net_init(&thd->net, 0);
> +  thd->main_security_ctx.master_access= ~0;
> +  thd->main_security_ctx.priv_user= 0;
> +  thd->real_id= pthread_self();
> +  /*
> +    Making this thread visible to SHOW PROCESSLIST is useful for
> +    troubleshooting a backup job (why does it stall etc).
> +  */
> +  pthread_mutex_lock(&LOCK_thread_count);
> +  threads.append(thd);
> +  pthread_mutex_unlock(&LOCK_thread_count);
> +  lex_start(thd);
> +  mysql_reset_thd_for_next_command(thd);
> +  DBUG_RETURN(thd);
> +}
> +
> +/**
> +  * @brief Creates a validity point by locking all tables.
> +  *
> +  * This method calls the open and lock tables for a driver. This method
> +  * is called from a separate thread and therefore can allow non-blocking
> +  * calls to open_and_lock_tables().
> +  */

Why not doing all what this function does inside the thread handler function? I 
see little reason to have separate function for that - seems that the only 
context in which it makes sense to call this function is inside the thread 
handler, right?

> +void lock_thread_tables(THD **lock_thd,
> +                        LOCK_STATE *lock_state,
> +                        TABLE_LIST *tables_in_backup)
> +{
> +  DBUG_ENTER("Default_backup::lock_tables");
> +
> +  /*
> +    Make sure we have tables to open.
> +  */
> +  if (!tables_in_backup)
> +  {
> +    DBUG_PRINT("info",("Online backup locking error no tables to lock"));
> +    *lock_state= LOCK_ERROR;
> +    DBUG_VOID_RETURN;
> +  }
> +
> +  /*
> +    As locking tables can be a long operation, we need to support
> +    cancellability during that time. So we publish our THD to the thread which
> +    created us.
> +  */
> +  if (open_and_lock_tables(*lock_thd, tables_in_backup))
> +  {
> +    DBUG_PRINT("info",("Online backup locking thread dying"));
> +    close_thread_tables(*lock_thd);
> +    DBUG_VOID_RETURN;
> +  }
> +
> +  /*
> +    Set lock state to acquired.
> +  */
> +  DBUG_PRINT("info",("Online backup locking thread got locks"));
> +  *lock_state= LOCK_ACQUIRED;
> +  DBUG_VOID_RETURN;
> +}
> +

> diff -Nrup a/sql/item_func.cc b/sql/item_func.cc
> --- a/sql/item_func.cc	2007-10-17 09:57:32 -04:00
> +++ b/sql/item_func.cc	2007-10-19 18:00:11 -04:00
> @@ -3404,7 +3404,9 @@ void debug_sync_point(const char* lock_n
>      Structure is now initialized.  Try to get the lock.
>      Set up control struct to allow others to abort locks
>    */
> -  THD_SET_PROC_INFO(thd, "User lock");
> +  char proc_info[128];
> +  my_sprintf(proc_info,(proc_info, "debug_sync_point: %s", lock_name));
> +  THD_SET_PROC_INFO(thd, proc_info);
>    thd->mysys_var->current_mutex= &LOCK_user_locks;
>    thd->mysys_var->current_cond=  &ull->cond;
>  

I don't like using internal function name in the proc_info string. Let's try 
something more user friendly like "Waiting on user lock '%s'" perhaps...

> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
> --- a/sql/share/errmsg.txt	2007-10-17 09:57:35 -04:00
> +++ b/sql/share/errmsg.txt	2007-10-19 18:00:18 -04:00
> @@ -6195,6 +6195,9 @@ ER_BACKUP_SEND_DATA_RETRY
>  ER_BACKUP_OPEN_TABLES
>          eng "Open and lock tables failed in %-.64s"
>  
> +ER_BACKUP_THREAD_INIT
> +        eng "Cannot initialize backup threads."
> +

I think this message should be more precise and indicate that the problem was 
with creating/initializing table locking thread for default backup driver.

>  ER_VIEW_NO_CREATION_CTX
>    eng "View `%-.64s`.`%-.64s` has no creation context"
>  ER_VIEW_INVALID_CREATION_CTX

Rafal