Re: bk commit into 6.0 tree (cbell:1.2589) BUG#34858

[Guilhem Bichot]

Hello Chuck,

On Fri, Mar 14, 2008 at 01:51:42PM +0100, cbell@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 6.0 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, 2008-03-14 08:51:11-04:00, cbell@mysql_cab_desk. +3 -0
>   BUG#34858 Server crashes by backup tests 
>   
>   Patch fixes problems encountered when merging runtime code changes
>   into main for online backup.
> 
>   mysql-test/t/disabled.def@stripped, 2008-03-14 08:51:06-04:00, cbell@mysql_cab_desk.
> +0 -5
>     BUG#34858 Server crashes by backup tests 
>     
>     Reenabled disabled tests.
> 
>   sql/backup/backup_progress.cc@stripped, 2008-03-14 08:51:07-04:00,
> cbell@mysql_cab_desk. +37 -5
>     BUG#34858 Server crashes by backup tests 
>     
>     Patch adds code to turn off binlogging for progress table writes.
>     Patch fixes bug caused by changes to handler.cc WRT current thread
>     access for writes.
> 
>   sql/backup/be_snapshot.cc@stripped, 2008-03-14 08:51:08-04:00, cbell@mysql_cab_desk. +1
> -0
>     BUG#34858 Server crashes by backup tests 
>     
>     Added code to perform commit of consistent read to avoid another
>     problem introduced from changes to handler.cc.
> 
> diff -Nrup a/mysql-test/t/disabled.def b/mysql-test/t/disabled.def
> --- a/mysql-test/t/disabled.def	2008-03-10 08:58:17 -04:00
> +++ b/mysql-test/t/disabled.def	2008-03-14 08:51:06 -04:00

> diff -Nrup a/sql/backup/backup_progress.cc b/sql/backup/backup_progress.cc
> --- a/sql/backup/backup_progress.cc	2008-02-26 15:18:12 -05:00
> +++ b/sql/backup/backup_progress.cc	2008-03-14 08:51:07 -04:00
> @@ -276,6 +276,9 @@ int update_online_backup_int_field(ulong
>    table= locking_thd->tables_in_backup->table;
>    table->use_all_columns();
>  
> +  THD *t= table->in_use;

t could be changed to a more explicit name. t can mean table,
transaction, thread, turtle...

> +  table->in_use= current_thd;
> +
>    if (find_online_backup_row(table, backup_id))
>    {
>      ret= close_backup_progress_table(locking_thd);
> @@ -290,12 +293,17 @@ int update_online_backup_int_field(ulong
>    table->field[fld]->store(value, TRUE);
>    table->field[fld]->set_notnull();
>  
> +  tmp_disable_binlog(current_thd);
> +
>    /*
>      Update the row.
>    */
>    if ((ret= table->file->ha_update_row(table->record[1],
> table->record[0])))
>      table->file->print_error(ret, MYF(0));
>  
> +  reenable_binlog(current_thd);

We should try to avoid multiple calls to current_thd (they imply a
call to pthread_getspecific()). In the above portion, there are three
new calls to current_thd; you can reduce that to one call if you cache
the resulting THD* into a local variable.

> +
> +  table->in_use= t;
>    ret= close_backup_progress_table(locking_thd);
>    DBUG_RETURN(ret);
>  }
> @@ -336,6 +344,9 @@ int update_online_backup_datetime_field(
>    table= locking_thd->tables_in_backup->table;
>    table->use_all_columns();
>  
> +  THD *t= table->in_use;
> +  table->in_use= current_thd;
> +
>    if (find_online_backup_row(table, backup_id))
>    {
>      ret= close_backup_progress_table(locking_thd);
> @@ -356,12 +367,17 @@ int update_online_backup_datetime_field(
>      table->field[fld]->store_time(&time, MYSQL_TIMESTAMP_DATETIME);
>    }
>  
> +  tmp_disable_binlog(current_thd);
> +
>    /*
>      Update the row.
>    */
>    if ((ret= table->file->ha_update_row(table->record[1],
> table->record[0])))
>      table->file->print_error(ret, MYF(0));
>  
> +  reenable_binlog(current_thd);
> +
> +  table->in_use= t;
>    ret= close_backup_progress_table(locking_thd);
>    DBUG_RETURN(ret);
>  }
> @@ -470,16 +486,19 @@ ulonglong report_ob_init(int process_id,
>        goto end;
>      table->field[ET_FIELD_COMMAND]->set_notnull();
>    }
> -  table->in_use= t;
>  
>    table->next_number_field=table->found_next_number_field;
>  
> +  tmp_disable_binlog(current_thd);
> +
>    /*
>      Write the row.
>    */
>    if ((ret= table->file->ha_write_row(table->record[0])))
>      table->file->print_error(ret, MYF(0));
>  
> +  reenable_binlog(current_thd);
> +
>    /*
>      Get last insert id for row.
>    */
> @@ -488,6 +507,7 @@ ulonglong report_ob_init(int process_id,
>  
>  end:
>  
> +  table->in_use= t;
>    ret= close_backup_progress_table(locking_thd);
>    /*
>      Record progress update.
> @@ -557,7 +577,8 @@ int report_ob_binlog_info(ulonglong back
>        goto end;
>      table->field[ET_FIELD_BINLOG_FILE]->set_notnull();
>    }
> -  table->in_use= t;
> +
> +  tmp_disable_binlog(current_thd);
>  
>    /*
>      Update the row.
> @@ -565,8 +586,11 @@ int report_ob_binlog_info(ulonglong back
>    if ((ret= table->file->ha_update_row(table->record[1],
> table->record[0])))
>      table->file->print_error(ret, MYF(0));
>  
> +  reenable_binlog(current_thd);
> +
>  end:
>  
> +  table->in_use= t;
>    ret= close_backup_progress_table(locking_thd);
>    DBUG_RETURN(ret);
>  }
> @@ -753,7 +777,8 @@ int report_ob_engines(ulonglong backup_i
>        goto end;
>      table->field[ET_FIELD_ENGINES]->set_notnull();
>    }
> -  table->in_use= t;
> +
> +  tmp_disable_binlog(current_thd);
>  
>    /*
>      Update the row.
> @@ -761,8 +786,11 @@ int report_ob_engines(ulonglong backup_i
>    if ((ret= table->file->ha_update_row(table->record[1],
> table->record[0])))
>      table->file->print_error(ret, MYF(0));
>  
> +  reenable_binlog(current_thd);
> +
>  end:
>  
> +  table->in_use= t;
>    ret= close_backup_progress_table(locking_thd);
>    DBUG_RETURN(ret);
>  }
> @@ -872,7 +900,6 @@ int report_ob_progress(ulonglong backup_
>        goto end;
>      table->field[ET_FIELD_PROG_NOTES]->set_notnull();
>    }
> -  table->in_use= t;
>  
>    if (start)
>    {
> @@ -899,14 +926,19 @@ int report_ob_progress(ulonglong backup_
>    table->field[ET_FIELD_PROG_ERROR_NUM]->store(error_num, TRUE);
>    table->field[ET_FIELD_PROG_ERROR_NUM]->set_notnull();
>  
> +  tmp_disable_binlog(current_thd);
> +
>    /*
>      Write the row.
>    */
>    if ((ret= table->file->ha_write_row(table->record[0])))
>      table->file->print_error(ret, MYF(0));
>  
> +  reenable_binlog(current_thd);
> +
>  end:
>  
> +  table->in_use= t;
>    ret= close_backup_progress_table(locking_thd);
>    DBUG_RETURN(ret);
>  }
> @@ -951,10 +983,10 @@ ulonglong sum_progress_rows(ulonglong ba
>          (table->field[ET_FIELD_PROG_ERROR_NUM]->val_int() == 0) &&
>          ((ulonglong)table->field[ET_FIELD_BACKUP_ID_FK]->val_int() ==
> backup_id))
>        size+= table->field[ET_FIELD_PROG_SIZE]->val_int();
> -  table->in_use= t;
>  
>    hdl->ha_rnd_end();
>  
> +  table->in_use= t;
>    close_backup_progress_table(locking_thd);
>    DBUG_RETURN(size);
>  }

Uh, the changing of table->in_use I don't think I can approve right
now. Normally, the code which opens tables (open_table() in
sql_base.cc) sets table->in_use properly, and after that there is no
reason to change table->in_use. So I'm skeptical about if
it's appropriate to pass the table around from thread to thread like
done here in update_online_backup_datetime_field() in the patch:

> +  THD *t= table->in_use;
> +  table->in_use= current_thd;
<cut>
> +  table->in_use= t;

So, I cannot approve and I'm running out of time to do a thorough
check of sql/backup/* files to make up my mind.
I suggest you find another person to do a real full review of the
patch. I'm sorry I cannot do it now, and I know it is urgent for you
to get it reviewed.

The assertion which you want to avoid is sensible, it says that either
table->in_use is not set or it should be equal to current_thd, in any
case table->in_use should not point to another thread.
So, passing tables from thread to thread may defeat the intention of
this assertion.

> diff -Nrup a/sql/backup/be_snapshot.cc b/sql/backup/be_snapshot.cc
> --- a/sql/backup/be_snapshot.cc	2007-11-29 23:22:05 -05:00
> +++ b/sql/backup/be_snapshot.cc	2008-03-14 08:51:08 -04:00
> @@ -121,6 +121,7 @@ result_t Backup::get_data(Buffer &buf)
>    if (locking_thd->lock_state == LOCK_SIGNAL)
>    {
>      locking_thd->lock_state= LOCK_DONE; // set lock done so destructor won't
> wait
> +    ha_autocommit_or_rollback(locking_thd->m_thd, 0);
>      end_active_trans(locking_thd->m_thd);
>      close_thread_tables(locking_thd->m_thd);

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /    Mr. Guilhem Bichot <guilhem@stripped>
 / /|_/ / // /\ \/ /_/ / /__   MySQL AB, Lead Software Engineer
/_/  /_/\_, /___/\___\_\___/   Bordeaux, France
       <___/   www.mysql.com