Rafal,
> Sorry that I haven't reviewed it by Friday.
No problem.
> 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.
Yes, this patch is OBE.
> 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 would be happy with this but it was decided to forego this patch in favor
of a better solution that gets us part of the way for the worklog you
mentioned.
> 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.
Got it. I will look at that.
> 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.
Ok.
>
> 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);
> > }
> >
> >
>
