Hi Chuck,
Sorry that I haven't reviewed it by Friday.
As I understand you propose a temporary solution to avoid violating the
assertion. The solution is to change table->in_use to point at current_thd and
thus make server code think that table operations are done from the thread which
opened them.
I agree with Guilhem's opinion that this is not a good way to proceed. If we use
hacks to go around assumptions made by the authors of the server code we are
asking for troubles -- sooner or later the code is going to break.
But we also have a WL for changing the logging code completely, so I think we
can accept this patch as an immediate solution for a showstopper. Perhaps you
should put some comment at the begging of the code informing about its status.
(Well, in an ideal world with infinite time resources we should wait for the WL
to be implemented and not push any temporary fixes, but I'm afraid we can't
afford doing that).
I have a suggestion. In many places in the code you temporarily disable and
re-enable binlog. But I think that binlog should always be disabled during
backup/restore operation. I mean disabled in the thread executing BACKUP/RESTORE
-- other threads should binlog or not as decided in WL#4209.
Therefore I suggest to write the backup progress code with assumption that the
binlog is disabled (this should be documented as a precondition) and modify the
kernel code to disable binloging. In the new version of the kernel (WL#4212)
this should be done inside Backup_restore_ctx::prepare() function.
Rafal
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
> @@ -35,8 +35,3 @@ backup_snapshot :BUG#34235 pending
> csv_alter_table : Bug#33696 2008-01-21 pcrews no .result file - bug allows NULL
> columns in CSV tables
> ctype_latin2_ch : BUG #33791 2008-01-18 mats Wrong ORDER BY with
> latin2_czech_cs
>
> -backup_errors : Bug#34858 2008-Feb-28 Server crashes by backup tests
> -backup_fkey : Bug#34858 2008-Feb-28 Server crashes by backup tests
> -backup_no_data : Bug#34858 2008-Feb-28 Server crashes by backup tests
> -backup_no_engine : Bug#34858 2008-Feb-28 Server crashes by backup tests
> -backup_objects : Bug#34858 2008-Feb-28 Server crashes by backup tests
=======================
> 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;
> + 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);
> +
> + 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);
> }
=======================
> 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);
> }
>
>
