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.
>
> 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.
> - 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.
>
> diff -Nrup a/mysql-test/r/backup_no_engine.result
> b/mysql-test/r/backup_no_engine.result
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/r/backup_no_engine.result 2007-09-12 20:36:52 +02:00
> @@ -0,0 +1,30 @@
> +DROP DATABASE IF EXISTS db;
> +CREATE DATABASE db;
> +CREATE TABLE db.t1 (a int, b char(32))
> +ENGINE=myisam;
> +BACKUP DATABASE db TO "db.backup";
> +Backup Summary
> + header = 12 bytes
> + meta-data = 120 bytes
> + data = 0 bytes
> + --------------
> + total 132 bytes
> +DROP DATABASE db;
> +CREATE DATABASE db;
> +RESTORE FROM "db.backup";
> +Restore Summary
> + header = 12 bytes
> + meta-data = 120 bytes
> + data = 0 bytes
> + --------------
> + total 132 bytes
> +SHOW TABLES IN db;
> +Tables_in_db
> +t1
> +SHOW CREATE TABLE db.t1;
> +Table Create Table
> +t1 CREATE TABLE `t1` (
> + `a` int(11) DEFAULT NULL,
> + `b` char(32) DEFAULT NULL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +DROP DATABASE db;
> Binary files a/mysql-test/std_data/bug30938.frm and
> b/mysql-test/std_data/bug30938.frm differ
> diff -Nrup a/mysql-test/t/backup_no_engine.test b/mysql-test/t/backup_no_engine.test
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/t/backup_no_engine.test 2007-09-12 20:36:52 +02:00
> @@ -0,0 +1,35 @@
> +# Test how online backup handles tables using non-existent storage engines
> +# The server used to crash in this situation (BUG#30938)
> +
> +# .frm file for a table using non-existent storage engine
> +--let $table_def=$MYSQL_TEST_DIR/std_data/bug30938.frm
> +--let $backup_out_path=$MYSQLTEST_VARDIR/master-data
> +
> +--disable_warnings
> +DROP DATABASE IF EXISTS db;
> +--enable_warnings
> +
> +CREATE DATABASE db;
> +CREATE TABLE db.t1 (a int, b char(32))
> +ENGINE=myisam;
> +
> +# copy description of a table using non-existent storage engine
> +--copy_file $table_def $MYSQLTEST_VARDIR/master-data/db/t2.frm
> +
> +# backup test database, the table with no storage engine will be ignored
> +BACKUP DATABASE db TO "db.backup";
> +
> +# wipe-out backed-up database
> +DROP DATABASE db;
> +CREATE DATABASE db;
> +
> +RESTORE FROM "db.backup";
> +
> +# check that the table with no engine was excluded
> +SHOW TABLES IN db;
> +# check that table t was correctly restored
> +SHOW CREATE TABLE db.t1;
> +
> +# clean up
> +DROP DATABASE db;
> +--remove_file $backup_out_path/db.backup
> diff -Nrup a/sql/backup/api_types.h b/sql/backup/api_types.h
> --- a/sql/backup/api_types.h 2007-06-28 16:34:48 +02:00
> +++ b/sql/backup/api_types.h 2007-09-12 20:36:51 +02:00
> @@ -136,6 +136,9 @@ class Table_ref
> return buf;
> }
>
> + const char* describe(describe_buf &buf) const
> + { return describe(buf,sizeof(buf)); }
> +
> protected:
>
> /*
> diff -Nrup a/sql/backup/backup_aux.h b/sql/backup/backup_aux.h
> --- a/sql/backup/backup_aux.h 2007-07-02 19:42:57 +02:00
> +++ b/sql/backup/backup_aux.h 2007-09-12 20:36:51 +02:00
> @@ -9,6 +9,12 @@ bool operator==(const String &s1, const
> return stringcmp(&s1,&s2) == 0;
> }
>
> +inline
> +bool operator!=(const String &s1, const String &s2)
> +{
> + return !(s1 == s2);
> +}
> +
> namespace backup {
>
> /**
> diff -Nrup a/sql/backup/backup_kernel.h b/sql/backup/backup_kernel.h
> --- a/sql/backup/backup_kernel.h 2007-08-20 10:37:06 +02:00
> +++ b/sql/backup/backup_kernel.h 2007-09-12 20:36:52 +02:00
> @@ -153,7 +153,10 @@ class Backup_info: public Archive_info,
> int snapshot_image_no; ///< Position of the snapshot image in @c images list,
> -1 if not used.
>
> Db_item* add_db(const backup::Db_ref&);
> - Table_item* add_table(Db_item&, const Table_ref&);
> + Table_item* add_table(const Table_ref&);
> +
> + /// Value returned by @c add_table if it decides that the table should be
> skipped.
> + static const Table_item *const skip_table;
>
> int add_db_items(Db_item&);
> int add_table_items(Table_item&);
> 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
> @@ -324,13 +324,22 @@ class Backup_info::Table_ref:
> {
> String m_db_name;
> String m_name;
> - const ::handlerton *m_hton;
> + ::TABLE *m_table;
>
> public:
>
> - Table_ref(TABLE*);
> - const ::handlerton *hton() const
> - { return m_hton; }
> + Table_ref(const Db_item&, const String&);
> + ~Table_ref()
> + { close(); }
> +
> + ::TABLE* open(THD*);
> + void close();
> +
> + ::handlerton* hton() const
> + { return m_table && m_table->s ? m_table->s->db_type() : NULL; }
> +
> + bool is_open() const
> + { return m_table != NULL; }
> };
>
> /**
> @@ -363,20 +372,22 @@ class Backup_info::Table_ref:
> int Backup_info::find_image(const Backup_info::Table_ref &tbl)
> {
> DBUG_ENTER("Backup_info::find_image");
> -
> - const ::handlerton *hton= tbl.hton();
> + // we assume that table has been opened already
> + DBUG_ASSERT(tbl.is_open());
> +
> Image_info *img;
> -
> + const ::handlerton *hton= tbl.hton();
> // If table has no handlerton something is really bad - we crash here
> DBUG_ASSERT(hton);
>
> - DBUG_PRINT("backup",("Adding table %s.%s using storage %s to archive%s",
> - tbl.db().name().ptr(),
> - tbl.name().ptr(),
> + Table_ref::describe_buf buf;
> +
> + DBUG_PRINT("backup",("Adding table %s using storage %s to archive%s",
> + tbl.describe(buf),
> ::ha_resolve_storage_engine_name(hton),
> hton->get_backup_engine ? " (has native backup)." :
> "."));
>
> - // Point 3: try existing images but not the default one.
> + // Point 3: try existing images but not the default or snapshot one.
>
> for (uint no=0; no < img_count && no < MAX_IMAGES ; ++no)
> {
> @@ -457,7 +468,6 @@ int Backup_info::find_image(const Backup
> if (img->accept(tbl,hton))
> DBUG_RETURN(ino); // table accepted
>
> - Table_ref::describe_buf buf;
> report_error(ER_BACKUP_NO_BACKUP_DRIVER,tbl.describe(buf,sizeof(buf)));
> DBUG_RETURN(-1);
> }
> @@ -476,6 +486,14 @@ namespace backup {
> // Returns tmp table containing records from a given I_S table
> TABLE* get_schema_table(THD *thd, ST_SCHEMA_TABLE *st);
>
> +/*
> + Backup_info::skip_table pointer is just for indicating that a table
> + added with Backup_info::add_table() was skipped. It should have value not
> + possible for regular pointers.
> + */
> +const Backup_info::Table_item *const
> +Backup_info::skip_table= reinterpret_cast<Backup_info::Table_item*>(1);
> +
> /**
> Create @c Backup_info structure and prepare it for populating with meta-data
> items.
> @@ -502,7 +520,6 @@ Backup_info::Backup_info(THD *thd):
> Backup_info::~Backup_info()
> {
> close();
> - //close_tables();
> m_state= DONE;
>
> Item_iterator it(*this);
> @@ -550,8 +567,7 @@ int Backup_info::save(OStream &s)
> }
>
> /**
> - Specialization of @c Db_ref which takes db name from @c LEX_STRING structure
> - or reads it from INFORMATION_SCHEMA.SCHEMATA table.
> + Specialization of @c backup::Db_ref with convenient constructors.
> */
>
> class Backup_info::Db_ref: public backup::Db_ref
> @@ -560,18 +576,15 @@ class Backup_info::Db_ref: public backup
>
> public:
>
> + Db_ref(const String &s): backup::Db_ref(m_name), m_name(s)
> + {}
> +
> Db_ref(const LEX_STRING &s):
> backup::Db_ref(m_name),
> m_name(s.str,s.length,::system_charset_info)
> {}
> -
> - Db_ref(TABLE *t): backup::Db_ref(m_name)
> - {
> - t->field[1]->val_str(&m_name);
> - }
> };
>
> -
> /**
> Add to archive all databases in the list.
> */
> @@ -620,9 +633,6 @@ int Backup_info::add_dbs(List< ::LEX_STR
> return backup::ERROR;
> }
>
> -static const LEX_STRING is_string = "information_schema";
> -static const LEX_STRING mysql_string = "mysql";
> -
> /**
> Add to archive all instance's databases (except the internal ones).
> */
> @@ -645,8 +655,6 @@ int Backup_info::add_all_dbs()
>
> // TODO: locking
>
> - const Db_ref is_db(is_string);
> - const Db_ref mysql_db(mysql_string);
> int res= 0;
>
> old_map= dbug_tmp_use_all_columns(db_table, db_table->read_set);
> @@ -660,14 +668,20 @@ int Backup_info::add_all_dbs()
>
> while (!ha->rnd_next(db_table->record[0]))
> {
> - Db_ref db(db_table);
> -
> - if (db==is_db || db==mysql_db)
> + String db_name;
> +
> + db_table->field[1]->val_str(&db_name);
> +
> + // skip internal databases
> + if (db_name == String("information_schema",&::my_charset_bin)
> + || db_name == String("mysql",&my_charset_bin))
> {
> - DBUG_PRINT("backup",(" Skipping internal database %s",db.name().ptr()));
> + DBUG_PRINT("backup",(" Skipping internal database %s",db_name.ptr()));
> continue;
> }
>
> + Db_ref db(db_name);
> +
> DBUG_PRINT("backup", (" Found database %s", db.name().ptr()));
>
> Db_item *it= add_db(db);
> @@ -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):
> + 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?
> + 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;
> @@ -841,34 +869,53 @@ int Backup_info::add_db_items(Db_item &d
> return res;
> }
>
> -/// Gets table identity from a row of I_S.TABLES table
> +/* Implementation of Backup_info::Table_ref */
>
> -Backup_info::Table_ref::Table_ref(TABLE *t):
> - backup::Table_ref(m_db_name,m_name)
> +Backup_info::Table_ref::Table_ref(const Db_item &db, const String &name):
> + backup::Table_ref(m_db_name,m_name), m_table(NULL)
> {
> - String engine_name;
> - plugin_ref plugin;
> -
> - t->field[1]->val_str(&m_db_name);
> - t->field[2]->val_str(&m_name);
> - t->field[4]->val_str(&engine_name);
> -
> - LEX_STRING name_lex;
> + m_db_name.append(db.name());
> + m_name.append(name);
> +}
>
> - name_lex.str= const_cast<char*>(engine_name.ptr());
> - name_lex.length= engine_name.length();
> +::TABLE* Backup_info::Table_ref::open(THD *thd)
> +{
> + if (is_open())
> + return m_table;
> +
> + char path[FN_REFLEN];
> + const char *db= m_db_name.ptr();
> + const char *name= m_name.ptr();
> + ::build_table_filename(path, sizeof(path), db, name, "", 0);
> + m_table= ::open_temporary_table(thd,path,db,name,FALSE /* don't link to
> thd->temporary_tables */);
> + return m_table;
> +}
>
> - plugin= ::ha_resolve_by_name(::current_thd,&name_lex);
> - m_hton= plugin ? plugin_data(plugin, handlerton*) : 0;
> +void Backup_info::Table_ref::close()
> +{
> + if (m_table)
> + {
> + // TODO: check if ::free_tmp_table() is not better
> + ::intern_close_table(m_table);
> + ::my_free(m_table, MYF(0));
> + }
> + m_table= NULL;
> + return;
> }
>
> /**
> - Add @c Table_item to archive's list of meta-data items.
> + Add table to archive's list of meta-data items.
> +
> + @pre Table should be opened.
> */
>
> Backup_info::Table_item*
> -Backup_info::add_table(Db_item&, const Table_ref &t)
> +Backup_info::add_table(const Table_ref &t)
> {
> + DBUG_ASSERT(t.is_open());
> +
> + // TODO: skip table if it is a tmp one
> +
> int no= find_image(t); // Note: find_image reports errors
>
> /*
> @@ -897,7 +944,7 @@ Backup_info::add_table(Db_item&, const T
> if (tno < 0 || TEST_ERROR)
> {
> Table_ref::describe_buf buf;
> -
> report_error(ER_BACKUP_NOT_ACCEPTED,img->name(),t.describe(buf,sizeof(buf)));
> + report_error(ER_BACKUP_NOT_ACCEPTED,img->name(),t.describe(buf));
> return NULL;
> }
>
> diff -Nrup a/sql/share/errmsg.txt b/sql/share/errmsg.txt
> --- a/sql/share/errmsg.txt 2007-08-26 08:11:37 +02:00
> +++ b/sql/share/errmsg.txt 2007-09-12 20:36:52 +02:00
> @@ -6132,6 +6132,10 @@ ER_BACKUP_LIST_DB_TABLES
> eng "Can't enumerate tables in database %-.64s"
> ER_BACKUP_SKIP_VIEW
> eng "Skipping view %-.64s in database %-.64s"
> +ER_BACKUP_NO_ENGINE
> + eng "Skipping table %-.64s since it has no valid storage engine"
> +ER_BACKUP_TABLE_OPEN
> + eng "Can't open table %-.64s"
> ER_BACKUP_READ_HEADER
> eng "Can't read backup archive preamble"
> ER_BACKUP_WRITE_HEADER
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe: http://lists.mysql.com/commits?unsub=1
>
regards,
Andrei
