Hi Rafal,
I like the changes you've made. While I wasn't able to reproduce the problem
on Windows (it just throws an error -- doesn't core), I see that your patch
makes the backup successful even when there are tables from storage engines
that do not exist. We may want to consider making the error message
accessible by the merlin code so that they can report it to the user in a
nice(r) way. Let's make that a TODO and not a must do for this bug report.
I'll annotate the bug report accordingly.
I did notice one problem, though. The backup_no_engine test failed. The
errors were:
main.backup_no_engine [ fail ]
mysqltest: At line 17: command "copy_file" failed with error 1
The result from queries just before the failure was:
DROP DATABASE IF EXISTS db;
CREATE DATABASE db;
CREATE TABLE db.t1 (a int, b char(32))
ENGINE=myisam;
The reason for this is because BK did not apply the patch to include the
file "bug30938.frm" for either Windows or Linux. I saw this once before
recently with a patch from Jeb. I don't know if BK is broken on my machines
(strange that it could be both of them) or if there is something wrong with
how BK is making the patches nowadays.
I got around this by using the file from the bug report to make the test
pass.
Other than that, I have a very minor change I'd like to have you take a look
at and implement or explain.
I am approving the patch assuming you will fix the minor wording issue
below.
Chuck
> -----Original Message-----
> From: rsomla@stripped [mailto:rsomla@stripped]
> Sent: Wednesday, September 12, 2007 2:37 PM
> To: commits@stripped
> Subject: bk commit into 5.2 tree (rafal:1.2601) BUG#30938
>
> 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,sizeo
> f(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.
> + */
The last sentence in the comment above doesn't make any sense to me. Can you
maybe rephrase it or explain what you mean?
> +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
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
>
