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.
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.
- 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.
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()));
+
+ 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;
+ report_error(ER_BACKUP_TABLE_OPEN,t.describe(buf));
+ 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
