Hi Andrei,
Thanks for the review. I have committed new patch taking into account your
comments. I left the error message as it was before, for the reasons I explain
below. Let me know if you accept the new patch.
Andrei Elkin wrote:
> Rafal, hello.
>
>
> I find that the patch addresses the reported problem.
> This part is okay.
>
> I believe that other parts dealing with some refactoring should be
> okay.
> However, I can not be certain as I really could not undestand some of
> logics (questions below).
> Besides that concern, I'd like you to notice that a list's elements in
> mysql style are to be separated by comma+space not just by comma.
> Please fix that.
>
>> Below is the list of changes that have just been committed into a local
>> 5.2 repository of rafal. When rafal 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, 2007-09-12 20:36:58+02:00, rafal@quant.(none) +8 -0
>> BUG#30938 (BACKUP DATABASE crashes server if a table with a unloaded
>> SE exists).
>>
>> Backup of a database failed if it contained a table using invalid storage
>> engine (e.g. plugin not loaded or unknown engine). This was because code
>> collecting list of tables in a database didn't exclude tables without a
>> valid storage engine and later kernel attempted to open such tables. This
>> patch fixes this by filtering out tables for which 'engine' field in
>> I_S.TABLES is NULL.
>>
>> mysql-test/r/backup_no_engine.result@stripped, 2007-09-12 20:36:52+02:00,
> rafal@quant.(none) +30 -0
>> Results for the test.
>>
>> mysql-test/r/backup_no_engine.result@stripped, 2007-09-12 20:36:52+02:00,
> rafal@quant.(none) +0 -0
>>
>> mysql-test/std_data/bug30938.frm@stripped, 2007-09-12 20:36:52+02:00,
> rafal@quant.(none) +201 -0
>> .frm file with table which uses no-existing engine. Used in
> backup_no_engine.test
>>
>> mysql-test/std_data/bug30938.frm@stripped, 2007-09-12 20:36:52+02:00,
> rafal@quant.(none) +0 -0
>>
>> mysql-test/t/backup_no_engine.test@stripped, 2007-09-12 20:36:52+02:00,
> rafal@quant.(none) +35 -0
>> Test that backup works if database contains a table with invalid storage
>> engine.
>>
>> mysql-test/t/backup_no_engine.test@stripped, 2007-09-12 20:36:52+02:00,
> rafal@quant.(none) +0 -0
>>
>> sql/backup/api_types.h@stripped, 2007-09-12 20:36:51+02:00, rafal@quant.(none) +3
> -0
>> Added another describe() method which checks buffer size using its type.
>>
>> sql/backup/backup_aux.h@stripped, 2007-09-12 20:36:51+02:00, rafal@quant.(none) +6
> -0
>
>> Added != operator for Strings.
>
> That's nice.
> I strongly recommend to move it in sql_string.h.
>
Done with pleasure :)
>> sql/backup/backup_kernel.h@stripped, 2007-09-12 20:36:52+02:00, rafal@quant.(none)
> +4 -1
>> Changed signature of Backup_info::add_table().
>>
>> sql/backup/sql_backup.cc@stripped, 2007-09-12 20:36:52+02:00, rafal@quant.(none)
> +121 -74
>
>> - Adding to Backup_info::Table_ref methods for opening and closing the
>> table.
>
> why? Is it needed for the bug. If not, please leave comments
> justifying this refactoring.
>
I added explanation in the comments for the new patch. In short - I open table
to get a pointer to its handlerton structure.
>> - Change code which reads database or table list from I_S tables. For
>> each record, first interesting fields are read and examined to
>> determine if it should be skipped or not. Then Db_ref or Table_ref
>> instance is created using data read from the record.
>> - When scanning tables in a database, skip tables with undefined storage
>> engine. Produce warning in error log.
>> - Handle the situation when Backup_info::add_table() informs that the
>> table should be skipped.
>> - Implement open()/close() methods for Backup_info::Table_ref.
>>
>> sql/share/errmsg.txt@stripped, 2007-09-12 20:36:52+02:00, rafal@quant.(none) +4
> -0
>
>> Add new messages.
>
> I wonder about one of them, below.
>
I explain my reasons below.
>> diff -Nrup a/sql/backup/sql_backup.cc b/sql/backup/sql_backup.cc
>> --- a/sql/backup/sql_backup.cc 2007-09-04 09:40:54 +02:00
>> +++ b/sql/backup/sql_backup.cc 2007-09-12 20:36:52 +02:00
>> @@ -782,48 +796,62 @@ int Backup_info::add_db_items(Db_item &d
>>
>> while (!ha->rnd_next(i_s_tables->record[0]))
>> {
>> - /*
>> - Check if it is table or view.
>> -
>> - FIXME: make it better when views are supported.
>> - */
>> + String db_name;
>> + String name;
>> String type;
>> -
>> + String engine;
>> +
>> + /*
>> + Read info about next table/view
>> +
>> + Note: this should be synchronized with the definition of
>> + INFORMATION_SCHEMA.TABLES table.
>> + */
>> + i_s_tables->field[1]->val_str(&db_name);
>> + i_s_tables->field[2]->val_str(&name);
>> i_s_tables->field[3]->val_str(&type);
>> + i_s_tables->field[4]->val_str(&engine);
>> +
>> + if (db_name != db.name())
>> + continue; // skip tables not from the given database
>>
>> - if (type == String("VIEW",&::my_charset_bin))
>> + // FIXME: right now, we handle only tables
>> + if (type != String("BASE TABLE",&::my_charset_bin))
>> {
>> - String name, db_name;
>> -
>> - i_s_tables->field[1]->val_str(&db_name);
>> - i_s_tables->field[2]->val_str(&name);
>> report_error(log_level::WARNING,ER_BACKUP_SKIP_VIEW,
>> name.c_ptr(),db_name.c_ptr());
>> continue;
>> }
>> -
>> - const Backup_info::Table_ref t(i_s_tables);
>>
>> - if (!t.hton())
>> + Backup_info::Table_ref t(db,name);
>> +
>> + if (engine.is_empty())
>> {
>> Table_ref::describe_buf buf;
>> - report_error(ER_NO_STORAGE_ENGINE,t.describe(buf,sizeof(buf)));
>> - goto error;
>> + report_error(log_level::WARNING,ER_BACKUP_NO_ENGINE,t.describe(buf));
>> + continue;
>> }
>>
>> - if (t.db() == db)
>> + DBUG_PRINT("backup", ("Found table %s for database %s",
>> + t.name().ptr(), t.db().name().ptr()));
>> +
>
> As I understand your code creates a new temporary table (why - i was
> asking above):
>
Yes, I open a tmp table here so that add_table(t) method below can use t.hton()
to get a pointer to table's handlerton structure.
>> + if (!t.open(m_thd))
>> {
>> - DBUG_PRINT("backup", ("Found table %s for database %s",
>> - t.name().ptr(), t.db().name().ptr()));
>> + Table_ref::describe_buf buf;
>
> in which case the only possible error is out of memory.
> However, the new error message is instead.
>
>> + report_error(ER_BACKUP_TABLE_OPEN,t.describe(buf));
>
> Why?
>
For the following reasons:
1. Compare these two error messages:
[ERROR] Backup: Out of memory.
and
[ERROR] Backup: Can't open table test.t1.
I think the second one is more informative and better describes the problem
which was encountered.
2. When I hit this error, what I know is that open_temporary_table() signalled
an error. Documentation of this function doesn't specify what kind of error it
was. Perhaps, in the current tree it can happen only because of lack of memory.
But who is going to guarantee that it will be the case in the future? I strongly
object against writing code which depends on such implicit assumptions. All
assumptions should be explicit and written down in the documentation. Otherwise
maintaining the code becomes a nightmare.
>> + goto error;
>> + }
>> + // add_table method selects/creates sub-image appropriate for storing given
> table
>> + Table_item *ti= add_table(t);
>> +
>> + if (!ti)
>> + goto error;
>>
>> - // add_table method selects/creates sub-image appropriate for storing
> given table
>> - Table_item *ti= add_table(db,t);
>> - if (!ti)
>> - goto error;
>> + if (ti == skip_table)
>> + continue;
>>
>> - if (add_table_items(*ti))
>> - goto error;
>> - }
>> + if (add_table_items(*ti))
>> + goto error;
>> }
>>
>> goto finish;
Best,
Rafal
