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

[Chuck Bell]

Rafal,

An explanation concerning the structure of the code...

I thought about moving the threading stuff into the be_default.cc file and
not splitting it out but it occurred to me that we are very likely going to
have to rewrite portions of this when we implement threading in general.
Also, the threading parts are written to be reusable and I anticipate we
could have the myisam driver use these methods as well. That is probably the
best argument for keeping the code as it is. I therefore decided to keep it
split into different sections for ease of reading the upper functions and
hiding the threading code itself from the driver code. 

If you feel strongly this is a bad idea I will change the code (it is just
copy and paste after all) but I would prefer to leave it the way it is to
unclutter the default driver with the threading stuff and make it easier for
future modifications. 

I will address your other comments and send out another patch later today.

Chuck

> -----Original Message-----
> From: Rafal Somla [mailto:rsomla@stripped] 
> Sent: Monday, October 22, 2007 9:44 AM
> To: cbell@stripped
> Cc: commits@stripped
> Subject: Re: bk commit into 5.2 tree (cbell:1.2610) BUG#31383
> 
> 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
> 
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    
> http://lists.mysql.com/commits?unsub=1
>