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-10-01 16:14:32+02:00, rafal@quant.(none) +9 -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 (Backup_archive::add_db_items() in
sql_backup.cc) 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.
Additionally, the way we access handlerton structure of table's storage engine
has been changed. Before, we located SE plugin sing its name and then read
handlerton pointer from the plugin:
plugin= ::ha_resolve_by_name(::current_thd,&name_lex);
m_hton= plugin ? plugin_data(plugin, handlerton*) : 0;
Now we open a table and read handlerton pointer from opened TABLE structure.
This is safer because it gives the open table code a chance to detect/deal with
non-existent or non-functional storage engines. If a table was opened
successfully we assume that its storage engine is fully functional.
Tables are opened just before they are added to the archive inside
Backup_archive::add_db_items(). Each table is closed after it has been added so
that only one table is open at a time.
mysql-test/r/backup_no_engine.result@stripped, 2007-10-01 16:13:20+02:00, rafal@quant.(none) +30 -0
Results for the test.
mysql-test/r/backup_no_engine.result@stripped, 2007-10-01 16:13:20+02:00, rafal@quant.(none) +0 -0
mysql-test/std_data/bug30938.frm@stripped, 2007-10-01 16:13:20+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-10-01 16:13:20+02:00, rafal@quant.(none) +0 -0
mysql-test/t/backup_no_engine.test@stripped, 2007-10-01 16:13:20+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-10-01 16:13:20+02:00, rafal@quant.(none) +0 -0
sql/backup/api_types.h@stripped, 2007-10-01 16:13:20+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-10-01 16:13:20+02:00, rafal@quant.(none) +0 -6
String operators moved to sql_string.h.
sql/backup/backup_kernel.h@stripped, 2007-10-01 16:13:20+02:00, rafal@quant.(none) +4 -1
- Changed signature of Backup_info::add_table(), now all needed data is
read fomr Table_ref instance.
- Added Backup_info::skip_table constant to be returned by add_table()
if the table should be ignored.
sql/backup/sql_backup.cc@stripped, 2007-10-01 16:13:20+02:00, rafal@quant.(none) +150 -74
- Adding to Backup_info::Table_ref methods for opening and closing the
table.
- Change implementation of Backup_info::Table_ref::hton() method to get
handlerton pointer from opened TABLE 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-10-01 16:13:20+02:00, rafal@quant.(none) +4 -0
Add new messages.
sql/sql_string.h@stripped, 2007-10-01 16:13:19+02:00, rafal@quant.(none) +13 -0
Added convenience operators for comparing Strings.
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-10-01 16:13:20 +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-10-01 16:13:20 +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-10-01 16:13:20 +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-10-01 16:13:20 +02:00
@@ -3,12 +3,6 @@
#include <backup/api_types.h>
-inline
-bool operator==(const String &s1, const String &s2)
-{
- return stringcmp(&s1,&s2) == 0;
-}
-
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-10-01 16:13:20 +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-10-01 16:13:20 +02:00
@@ -324,13 +324,29 @@ 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();
+
+ /**
+ Return pointer to @c handlerton structure of table's storage engine.
+
+ @return @c NULL if table has not been opened or pointer to the @c handlerton
+ structure of table's storage engine.
+ */
+ ::handlerton* hton() const
+ { return m_table && m_table->s ? m_table->s->db_type() : NULL; }
+
+ /// Check if table has been opened.
+ bool is_open() const
+ { return m_table != NULL; }
};
/**
@@ -363,20 +379,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 +475,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 +493,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 +527,6 @@ Backup_info::Backup_info(THD *thd):
Backup_info::~Backup_info()
{
close();
- //close_tables();
m_state= DONE;
Item_iterator it(*this);
@@ -550,8 +574,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 +583,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 +640,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 +662,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 +675,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 +803,66 @@ 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()));
+
+ // We need to open table for add_table() method below
+ 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);
+
+ // Close table to free resources
+ t.close();
+
+ 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 +880,71 @@ 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();
+/**
+ Open table and create corresponding @c TABLE structure.
+
+ A pointer to opened @c TABLE structure is stored in @c m_table member. The
+ structure is owned by @c Table_ref object, to destroy it call @c close()
+ method.
+
+ This method does nothing if table has been already opened.
+
+ @return Pointer to the opened @c TABLE structure or @c NULL if operation was
+ not successful.
+ */
+::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;
+/**
+ Close previously opened table.
+
+ Closes table and frees allocated resources. Can be called even when table
+ has not been opened, in which case it does nothing.
+ */
+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 +973,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-10-01 16:13:20 +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
diff -Nrup a/sql/sql_string.h b/sql/sql_string.h
--- a/sql/sql_string.h 2007-01-24 18:57:01 +01:00
+++ b/sql/sql_string.h 2007-10-01 16:13:19 +02:00
@@ -370,3 +370,16 @@ static inline bool check_if_only_end_spa
{
return str+ cs->cset->scan(cs, str, end, MY_SEQ_SPACES) == end;
}
+
+inline
+bool operator==(const String &s1, const String &s2)
+{
+ return stringcmp(&s1,&s2) == 0;
+}
+
+inline
+bool operator!=(const String &s1, const String &s2)
+{
+ return !(s1 == s2);
+}
+
| Thread |
|---|
| • bk commit into 5.2 tree (rafal:1.2601) BUG#30938 | rsomla | 1 Oct |
