| List: | Commits | « Previous MessageNext Message » | |
| From: | Oystein.Grovlen | Date: | October 2 2008 4:35pm |
| Subject: | bzr commit into mysql-6.0-backup branch (oystein.grovlen:2700) Bug#39089 WL#4384 | ||
| View as plain text | |||
#At file:///home/oysteing/mysql/mysql-6.0-backup-1/ 2700 oystein.grovlen@stripped 2008-10-02 BUG#39089/WL#4384 Fix missing error handling in backup code. Make sure errors returned from functions are reports are handled and/or logged. modified: sql/Makefile.am sql/backup/backup_aux.h sql/backup/backup_info.cc sql/backup/backup_kernel.h sql/backup/data_backup.cc sql/backup/image_info.cc sql/backup/image_info.h sql/backup/kernel.cc sql/backup/logger.cc sql/backup/logger.h sql/backup/stream.cc sql/backup/stream.h sql/mdl.cc sql/share/errmsg.txt per-file messages: sql/Makefile.am Avoid warning about white-space after '\' sql/backup/backup_aux.h Report failures to allocate locks. sql/backup/backup_info.cc Report&handle allocation errors. Require Global_iterator constructor take Backup_info parameter instead of Image_info in order to be able to log errors. sql/backup/backup_kernel.h Add Backup_restore_ctx::log_error to log errors to backup logs without pushing them on the server's warning stack. Made unlock_tables() void since it never returns errors. sql/backup/data_backup.cc Detect&Report error. Make Block:writer:drop_buf() void since it never errors. Add comments to explain where error handling is not necessary. sql/backup/image_info.cc Add comments to explain where error handling is not needed. sql/backup/image_info.h Add init() function to Image_info::Iterator for initialization code that may generate errors. Make functions that never return errors void. Add comments on error handling. sql/backup/kernel.cc Detect and handle previously undetected errors. Add comments on error handling. Add Backup_restore_ctx::log_error to log errors to backup logs without pushing them on the server's warning stack. sql/backup/logger.cc Add method Logger::push_error to set whether errors should be pushed to warning stack or not. sql/backup/logger.h Add method Logger::push_error to set whether errors should be pushed to warning stack or not. sql/backup/stream.cc Add error handling for close methods. sql/backup/stream.h Close methods should return bool in order to be able to report errors. sql/mdl.cc More specific return value documentation for mdl_alloc_lock(). === modified file 'sql/Makefile.am' --- a/sql/Makefile.am 2008-09-16 08:34:30 +0000 +++ b/sql/Makefile.am 2008-10-02 14:35:29 +0000 @@ -135,7 +135,7 @@ mysqld_SOURCES = sql_lex.cc sql_handler. sql_plugin.cc sql_binlog.cc \ sql_builtin.cc sql_tablespace.cc partition_info.cc \ sql_servers.cc sql_audit.cc sha2.cc \ - ddl_blocker.cc si_objects.cc si_logs.cc \ + ddl_blocker.cc si_objects.cc si_logs.cc \ event_parse_data.cc mdl.cc transaction.cc if HAVE_DTRACE === modified file 'sql/backup/backup_aux.h' --- a/sql/backup/backup_aux.h 2008-08-21 19:28:49 +0000 +++ b/sql/backup/backup_aux.h 2008-10-02 14:35:29 +0000 @@ -6,11 +6,6 @@ @brief Auxiliary declarations used in online backup code. - @todo Fix error detection in places marked with "FIXME: detect errors...". - These are places where functions or methods are called and if they can - report errors it should be detected and appropriate action taken. If callee - never reports errors or we want to ignore errors, a comment explaining this - should be added. */ typedef st_plugin_int* storage_engine_ref; @@ -140,8 +135,8 @@ class String: public ::String }; inline -void set_table_list(TABLE_LIST &tl, const Table_ref &tbl, - thr_lock_type lock_type, MEM_ROOT *mem) +int set_table_list(TABLE_LIST &tl, const Table_ref &tbl, + thr_lock_type lock_type, MEM_ROOT *mem) { DBUG_ASSERT(mem); @@ -149,8 +144,12 @@ void set_table_list(TABLE_LIST &tl, cons tl.db= const_cast<char*>(tbl.db().name().ptr()); tl.lock_type= lock_type; - // FIXME: detect errors (if NULL returned). tl.mdl_lock_data= mdl_alloc_lock(0, tl.db, tl.table_name, mem); + if (!tl.mdl_lock_data) // Failed to allocate lock + { + return 1; + } + return 0; } inline @@ -165,7 +164,10 @@ TABLE_LIST* mk_table_list(const Table_re return NULL; bzero(ptr, sizeof(TABLE_LIST)); - set_table_list(*ptr, tbl, lock_type, mem); + if (set_table_list(*ptr, tbl, lock_type, mem)) // Failed to allocate lock + { + return NULL; + } return ptr; } === modified file 'sql/backup/backup_info.cc' --- a/sql/backup/backup_info.cc 2008-09-16 16:09:18 +0000 +++ b/sql/backup/backup_info.cc 2008-10-02 14:35:29 +0000 @@ -5,16 +5,6 @@ implements algorithm for selecting backup engine used to backup given table. - @todo Fix error detection in places marked with "FIXME: detect errors...". - These are places where functions or methods are called and if they can - report errors it should be detected and appropriate action taken. If callee - never reports errors or we want to ignore errors, a comment explaining this - should be added. - - @todo Fix error logging in places marked with "FIXME: error logging...". In - these places it should be decided if and how the error should be shown to the - user. If an error message should be logged, it can happen either in the place - where error was detected or somewhere up the call stack. */ #include "../mysql_priv.h" @@ -317,14 +307,16 @@ Backup_info::Backup_info(Backup_restore_ bzero(m_snap, sizeof(m_snap)); - // FIXME: detect errors if reported. - // FIXME: error logging. - hash_init(&ts_hash, &::my_charset_bin, 16, 0, 0, - Ts_hash_node::get_key, Ts_hash_node::free, MYF(0)); - // FIXME: detect errors if reported. - // FIXME: error logging. - hash_init(&dep_hash, &::my_charset_bin, 16, 0, 0, - Dep_node::get_key, Dep_node::free, MYF(0)); + if (hash_init(&ts_hash, &::my_charset_bin, 16, 0, 0, + Ts_hash_node::get_key, Ts_hash_node::free, MYF(0)) + || + hash_init(&dep_hash, &::my_charset_bin, 16, 0, 0, + Dep_node::get_key, Dep_node::free, MYF(0))) + { + // Allocation failed. Error has been reported, but not logged to backup logs + m_ctx.log_error(ER_OUT_OF_RESOURCES); + return; + } /* Create nodata, default, and CS snapshot objects and add them to the @@ -332,35 +324,56 @@ Backup_info::Backup_info(Backup_restore_ element on that list, as a "catch all" entry. */ - snap= new Nodata_snapshot(m_ctx); // reports errors - - // FIXME: error logging (in case snap could not be allocated). - if (!snap || !snap->is_valid()) + snap= new Nodata_snapshot(m_ctx); // logs errors + if (!snap) + { + m_ctx.fatal_error(ER_OUT_OF_RESOURCES); return; + } - // FIXME: detect errors if reported. - // FIXME: error logging. - snapshots.push_back(snap); + if (!snap->is_valid()) + return; // Error has been logged by Nodata_snapshot constructor - snap= new CS_snapshot(m_ctx); // reports errors + if (snapshots.push_back(snap)) + { + // Allocation failed. Error has been reported, but not logged to backup logs + m_ctx.log_error(ER_OUT_OF_RESOURCES); + return; + } - // FIXME: error logging (in case snap could not be allocated). - if (!snap || !snap->is_valid()) + snap= new CS_snapshot(m_ctx); // logs errors + if (!snap) + { + m_ctx.fatal_error(ER_OUT_OF_RESOURCES); return; + } - // FIXME: detect errors if reported. - // FIXME: error logging. - snapshots.push_back(snap); + if (!snap->is_valid()) + return; // Error has been logged by CS_snapshot constructor - snap= new Default_snapshot(m_ctx); // reports errors + if (snapshots.push_back(snap)) + { + // Allocation failed. Error has been reported, but not logged to backup logs + m_ctx.log_error(ER_OUT_OF_RESOURCES); + return; // Error has been logged + } - // FIXME: error logging (in case snap could not be allocated). - if (!snap || !snap->is_valid()) + snap= new Default_snapshot(m_ctx); // logs errors + if (!snap) + { + m_ctx.fatal_error(ER_OUT_OF_RESOURCES); return; + } + + if (!snap->is_valid()) + return; // Error has been logged by Default_snapshot constructor - // FIXME: detect errors if reported. - // FIXME: error logging. - snapshots.push_back(snap); + if (snapshots.push_back(snap)) + { + // Allocation failed. Error has been reported, but not logged to backup logs + m_ctx.log_error(ER_OUT_OF_RESOURCES); + return; // Error has been logged + } m_state= CREATED; } @@ -1297,7 +1310,9 @@ class Backup_info::Global_iterator public: - Global_iterator(const Image_info&); + Global_iterator(const Backup_info&); + + int init(); private: @@ -1306,14 +1321,24 @@ class Backup_info::Global_iterator }; inline -Backup_info::Global_iterator::Global_iterator(const Image_info &info) +Backup_info::Global_iterator::Global_iterator(const Backup_info &info) :Iterator(info), mode(TABLESPACES), m_it(NULL), m_obj(NULL) +{} + + +inline +int Backup_info::Global_iterator::init() { - // FIXME: detect errors (if NULL returned). m_it= m_info.get_tablespaces(); - next(); // Note: next() doesn't report errors. -} + if (!m_it) + { + const Backup_info* info= static_cast<const Backup_info*>(&m_info); + return info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES); + } + next(); // Never errors + return 0; +} inline backup::Image_info::Obj* @@ -1351,6 +1376,14 @@ Backup_info::Global_iterator::next() // We have finished enumerating tablespaces, move on to databases. mode= DATABASES; m_it= m_info.get_dbs(); + if (!m_it) + { + const Backup_info* info= static_cast<const Backup_info*>(&m_info); + info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES); + mode= DONE; + return FALSE; + } + m_obj= (*m_it)++; return m_obj != NULL; @@ -1426,11 +1459,33 @@ bool Backup_info::Perdb_iterator::next() /// Wrapper to return global iterator. backup::Image_info::Iterator* Backup_info::get_global() const { - return new Global_iterator(*this); + Global_iterator* it = new Global_iterator(*this); + if (it == NULL) + { + m_ctx.fatal_error(ER_OUT_OF_RESOURCES); + return NULL; + } + if (it->init()) // Error has been logged + { + return NULL; + } + + return it; } /// Wrapper to return iterator for per-database objects. backup::Image_info::Iterator* Backup_info::get_perdb() const { - return new Perdb_iterator(*this); + Perdb_iterator* it = new Perdb_iterator(*this); + if (it == NULL) + { + m_ctx.fatal_error(ER_OUT_OF_RESOURCES); + return NULL; + } + if (it->init()) // Error has been logged + { + return NULL; + } + + return it; } === modified file 'sql/backup/backup_kernel.h' --- a/sql/backup/backup_kernel.h 2008-09-02 09:04:39 +0000 +++ b/sql/backup/backup_kernel.h 2008-10-02 14:35:29 +0000 @@ -76,6 +76,7 @@ class Backup_restore_ctx: public backup: int do_backup(); int do_restore(); int fatal_error(int, ...); + int log_error(int, ...); int close(); @@ -127,7 +128,7 @@ class Backup_restore_ctx: public backup: bool m_tables_locked; int lock_tables_for_restore(); - int unlock_tables(); + void unlock_tables(); int report_stream_open_failure(int open_error, const LEX_STRING *location); === modified file 'sql/backup/data_backup.cc' --- a/sql/backup/data_backup.cc 2008-09-30 07:51:48 +0000 +++ b/sql/backup/data_backup.cc 2008-10-02 14:35:29 +0000 @@ -7,17 +7,6 @@ drivers and protocols to create snapshot of the data stored in the tables being backed up. - @todo Fix error detection in places marked with "FIXME: detect errors...". - These are places where functions or methods are called and if they can - report errors it should be detected and appropriate action taken. If callee - never reports errors or we want to ignore errors, a comment explaining this - should be added. - - @todo Fix error logging in places marked with "FIXME: error logging...". In - these places it should be decided if and how the error should be shown to the - user. If an error message should be logged, it can happen either in the place - where error was detected or somewhere up the call stack. - @todo Implement better scheduling strategy in Scheduler::step @todo Add error reporting in the scheduler and elsewhere @todo If an error from driver is ignored (and operation retried) leave trace @@ -104,7 +93,7 @@ class Block_writer result_t get_buf(Buffer &); result_t write_buf(const Buffer&); - result_t drop_buf(Buffer&); + void drop_buf(Buffer&); Block_writer(byte, size_t, Output_stream&); ~Block_writer(); @@ -477,9 +466,14 @@ int write_table_data(THD* thd, Backup_in if (init_size > max_init_size) max_init_size= init_size; - // FIXME: detect errors if reported. - // FIXME: error logging. - inactive.push_back(p); + if (inactive.push_back(p)) + { + /* Allocation failed. + Error has been reported, but not logged to backup logs. + */ + info.m_ctx.log_error(ER_OUT_OF_RESOURCES); + goto error; + } } } @@ -599,9 +593,11 @@ int write_table_data(THD* thd, Backup_in Save binlog information for point in time recovery on restore. */ if (mysql_bin_log.is_open()) - // FIXME: detect errors if reported. - // FIXME: error logging. - mysql_bin_log.get_current_log(&binlog_pos); + if (mysql_bin_log.get_current_log(&binlog_pos)) + { + info.m_ctx.fatal_error(ER_BACKUP_BINLOG); + goto error; + } /* Save VP creation time. @@ -788,8 +784,7 @@ int Scheduler::step() break; case backup_state::DONE: - // FIXME: detect errors if reported. - p->end(); + res= p->end(); // Logs errors, fall-through to error handling below case backup_state::ERROR: remove_pump(p); // Note: never errors. @@ -1014,11 +1009,15 @@ Backup_pump::Backup_pump(Snapshot_info & { DBUG_ASSERT(snap.m_num > 0); m_buf.data= NULL; - // FIXME: detect errors if reported. - bitmap_init(&m_closed_streams, + if (bitmap_init(&m_closed_streams, NULL, 1 + snap.table_count(), - FALSE); // not thread safe + FALSE)) // not thread safe + { + state= backup_state::ERROR; // Created object will be invalid + return; + } + m_name= snap.name(); if (ERROR == snap.get_backup_driver(m_drv) || !m_drv) state= backup_state::ERROR; @@ -1253,10 +1252,8 @@ int Backup_pump::pump(size_t *howmuch) if (m_buf.size > 0) mode= WRITING; - else - // FIXME: detect errors (perhaps ensure that drop_buf never errors). - // FIXME: error logging. - m_bw.drop_buf(m_buf); + else + m_bw.drop_buf(m_buf); // Never errors break; @@ -1274,9 +1271,7 @@ int Backup_pump::pump(size_t *howmuch) state= backup_state::DONE; case BUSY: - // FIXME: detect errors (perhaps ensure that drop_buf never errors). - // FIXME: error logging. - m_bw.drop_buf(m_buf); + m_bw.drop_buf(m_buf); // Never errors m_buf_head=NULL; // thus a new request will be made } @@ -1531,9 +1526,7 @@ int restore_table_data(THD *thd, Restore bad_drivers.append(","); bad_drivers.append(info.m_snap[n]->name()); } - // FIXME: detect errors. - // FIXME: error logging. - drv[n]->free(); + drv[n]->free(); // Never errors } if (!bad_drivers.is_empty()) @@ -1551,9 +1544,7 @@ int restore_table_data(THD *thd, Restore if (!drv[n]) continue; - // FIXME: detect errors. - // FIXME: error logging (perhaps we don't want to report errors here). - drv[n]->free(); + drv[n]->free(); // Never errors } DBUG_RETURN(backup::ERROR); @@ -1651,13 +1642,12 @@ Block_writer::write_buf(const Buffer &bu method can return it to the buffer pool so that it can be reused for other transfers. */ -Block_writer::result_t +void Block_writer::drop_buf(Buffer &buf) { buf.data= NULL; buf.size= 0; taken= FALSE; - return OK; } } // backup namespace === modified file 'sql/backup/image_info.cc' --- a/sql/backup/image_info.cc 2008-08-20 13:23:10 +0000 +++ b/sql/backup/image_info.cc 2008-10-02 14:35:29 +0000 @@ -8,16 +8,6 @@ @brief Implements @c Image_info class and friends. - @todo Fix error detection in places marked with "FIXME: detect errors...". - These are places where functions or methods are called and if they can - report errors it should be detected and appropriate action taken. If callee - never reports errors or we want to ignore errors, a comment explaining this - should be added. - - @todo Fix error logging in places marked with "FIXME: error logging...". In - these places it should be decided if and how the error should be shown to the - user. If an error message should be logged, it can happen either in the place - where error was detected or somewhere up the call stack. */ namespace backup { @@ -25,8 +15,7 @@ namespace backup { Image_info::Image_info() :data_size(0), m_table_count(0), m_dbs(16, 16), m_ts_map(16,16) { - // FIXME: detect errors if reported. - init_alloc_root(&mem_root, 4 * 1024, 0); + init_alloc_root(&mem_root, 4 * 1024, 0); // Never errors /* initialize st_bstream_image_header members */ @@ -308,10 +297,8 @@ Image_info::add_table(Db &db, const ::St if (snap.add_table(*t, pos)) // reports errors return NULL; - - // FIXME: error logging. - if (db.add_table(*t)) - return NULL; + + db.add_table(*t); // Never errors if (!snap.m_num) snap.m_num= add_snapshot(snap); // reports errors === modified file 'sql/backup/image_info.h' --- a/sql/backup/image_info.h 2008-10-01 15:19:53 +0000 +++ b/sql/backup/image_info.h 2008-10-02 14:35:29 +0000 @@ -4,10 +4,6 @@ /** @file - @todo Fix error logging in places marked with "FIXME: error logging...". In - these places it should be decided if and how the error should be shown to the - user. If an error message should be logged, it can happen either in the place - where error was detected or somewhere up the call stack. */ #include <si_objects.h> @@ -424,7 +420,7 @@ class Image_info::Db obs::Obj* materialize(uint ver, const ::String &sdata); result_t add_obj(Dbobj&, ulong pos); Dbobj* get_obj(ulong pos) const; - result_t add_table(Table&); + void add_table(Table&); const char* describe(describe_buf&) const; private: @@ -559,6 +555,15 @@ class Image_info::Iterator Iterator(const Image_info &info); virtual ~Iterator(); + + /** + Initialize the iterator after construction. + Subclasses need to implement this if initialization may generate errors. + + @returns 0 if success. Otherwise, error code. + */ + virtual int init() { return 0; } + Obj* operator++(int); protected: @@ -836,28 +841,52 @@ void Image_info::save_binlog_pos(const : flags|= BSTREAM_FLAG_BINLOG; } -/// Returns an iterator enumerating all databases stored in backup catalogue. +/** + Returns an iterator enumerating all databases stored in backup catalogue. + + @returns Pointer to @c Image_info::Db_iterator or NULL if allocation fails. + */ inline Image_info::Db_iterator* Image_info::get_dbs() const { - // FIXME: error logging (in case allocation fails). - return new Db_iterator(*this); + Db_iterator* it = new Db_iterator(*this); + + if (it && it->init()) // Initialization failed + it= NULL; + + return it; // Error logging context not available, caller must handle NULL } -/// Returns an iterator enumerating all tablespaces stored in backup catalogue. +/** + Returns an iterator enumerating all tablespaces stored in backup catalogue. + + @returns Pointer to @c Image_info::Ts_iterator or NULL if allocation fails. + */ inline Image_info::Ts_iterator* Image_info::get_tablespaces() const { - // FIXME: error logging (in case allocation fails). - return new Ts_iterator(*this); + Ts_iterator* it = new Ts_iterator(*this); + + if (it && it->init()) // Initialization failed + it= NULL; + + return it; // Error logging context not available, caller must handle NULL } -/// Returns an iterator enumerating all objects in a given database. +/** + Returns an iterator enumerating all objects in a given database. + + @returns Pointer to @c Image_info::Dbobj_iterator or NULL if allocation fails. + */ inline Image_info::Dbobj_iterator* Image_info::get_db_objects(const Db &db) const { - // FIXME: error logging (in case allocation fails). - return new Dbobj_iterator(*this, db); + Dbobj_iterator* it = new Dbobj_iterator(*this, db); + + if (it && it->init()) // Initialization failed + it= NULL; + + return it; // Error logging context not available, caller must handle NULL } /******************************************************************** @@ -1065,7 +1094,7 @@ obs::Obj* Image_info::Dbobj::materialize The table is appended to database's table list. */ inline -result_t Image_info::Db::add_table(Table &tbl) +void Image_info::Db::add_table(Table &tbl) { tbl.next_table= NULL; @@ -1076,8 +1105,6 @@ result_t Image_info::Db::add_table(Table last_table->next_table= &tbl; last_table= &tbl; } - - return OK; } /** === modified file 'sql/backup/kernel.cc' --- a/sql/backup/kernel.cc 2008-09-30 07:51:48 +0000 +++ b/sql/backup/kernel.cc 2008-10-02 14:35:29 +0000 @@ -52,17 +52,6 @@ } // if code jumps here, context destructor will do the clean-up automatically @endcode - @todo Fix error detection in places marked with "FIXME: detect errors...". - These are places where functions or methods are called and if they can - report errors it should be detected and appropriate action taken. If callee - never reports errors or we want to ignore errors, a comment explaining this - should be added. - - @todo Fix error logging in places marked with "FIXME: error logging...". In - these places it should be decided if and how the error should be shown to the - user. If an error message should be logged, it can happen either in the place - where error was detected or somewhere up the call stack. - @todo Use internal table name representation when passing tables to backup/restore drivers. @todo Handle other types of meta-data in Backup_info methods. @@ -286,8 +275,10 @@ int send_error(Backup_restore_ctx &log, /** Send positive reply after a backup/restore operation. - Currently the id of the operation is returned. It can be used to select - correct entries form the backup progress tables. + Currently the id of the operation is returned to the client. It can + be used to select correct entries from the backup progress tables. + + @returns 0 on success, error code otherwise. */ int send_reply(Backup_restore_ctx &context) { @@ -300,32 +291,37 @@ int send_reply(Backup_restore_ctx &conte /* Send field list. */ - // FIXME: detect errors if reported. - // FIXME: error logging. - field_list.push_back(new Item_empty_string(STRING_WITH_LEN("backup_id"))); - protocol->send_result_set_metadata(&field_list, Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF); + if (field_list.push_back(new Item_empty_string(STRING_WITH_LEN("backup_id")))) + { + goto err; + } + if (protocol->send_result_set_metadata(&field_list, + Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF)) + { + goto err; + } /* Send field data. */ - // FIXME: detect errors if reported. - // FIXME: error logging. - protocol->prepare_for_resend(); - // FIXME: detect errors if reported. - // FIXME: error logging. - llstr(context.op_id(), buf); - // FIXME: detect errors if reported. - // FIXME: error logging. - protocol->store(buf, system_charset_info); - // FIXME: detect errors if reported. - // FIXME: error logging. - protocol->write(); - - // FIXME: detect errors if reported. - // FIXME: error logging. - my_eof(context.thd()); - context.report_cleanup(); + protocol->prepare_for_resend(); // Never errors + llstr(context.op_id(), buf); // Never errors + if (protocol->store(buf, system_charset_info)) + { + goto err; + } + if (protocol->write()) + { + goto err; + } + my_eof(context.thd()); // Never errors + context.report_cleanup(); // Never errors DBUG_RETURN(0); + + err: + DBUG_RETURN(context.fatal_error(ER_BACKUP_SEND_REPLY, + context.m_type == backup::Logger::BACKUP + ? "BACKUP" : "RESTORE")); } @@ -409,13 +405,10 @@ int Backup_restore_ctx::prepare(LEX_STRI // Prepare error reporting context. - // FIXME: detect errors if reported. - // FIXME: error logging. - mysql_reset_errors(m_thd, 0); + mysql_reset_errors(m_thd, 0); // Never errors m_thd->no_warnings_for_error= FALSE; - // FIXME: detect errors if reported. - // FIXME: error logging. - save_errors(); + + save_errors(); // Never errors /* @@ -566,7 +559,7 @@ Backup_restore_ctx::prepare_for_backup(S Create backup catalogue. */ - Backup_info *info= new Backup_info(*this); // reports errors + Backup_info *info= new Backup_info(*this); // Logs errors if (!info) { @@ -575,7 +568,7 @@ Backup_restore_ctx::prepare_for_backup(S } if (!info->is_valid()) - return NULL; + return NULL; // Error has been logged by Backup_Info constructor /* If binlog is enabled, set BSTREAM_FLAG_BINLOG in the header to indicate @@ -743,14 +736,14 @@ int Backup_restore_ctx::lock_tables_for_ backup::Image_info::Table *tbl= snap->get_table(t); DBUG_ASSERT(tbl); // All tables should be present in the catalogue. - // FIXME: detect errors. Don't assert here but report error instead. - // FIXME: error logging. TABLE_LIST *ptr= backup::mk_table_list(*tbl, TL_WRITE, m_thd->mem_root); - DBUG_ASSERT(ptr); + if (!ptr) + { + // Error has been reported, but not logged to backup logs + return log_error(ER_OUT_OF_RESOURCES); + } - // FIXME: detect errors if reported. - // FIXME: error logging. - tables= backup::link_table_list(*ptr, tables); + tables= backup::link_table_list(*ptr, tables); // Never errors tbl->m_table= ptr; } } @@ -775,20 +768,54 @@ int Backup_restore_ctx::lock_tables_for_ /** Unlock tables which were locked by @c lock_tables_for_restore. */ -int Backup_restore_ctx::unlock_tables() +void Backup_restore_ctx::unlock_tables() { // Do nothing if tables are not locked. if (!m_tables_locked) - return 0; + return; DBUG_PRINT("restore",("unlocking tables")); - close_thread_tables(m_thd); + close_thread_tables(m_thd); // Never errors m_tables_locked= FALSE; - return 0; + return; +} + + +/** + Report error and move context object into error state without pushing the + error on the server's warning stack. + + Similar to @c fatal_error, but does not push the error on the + server's warning stack. To be used when an error is reported from a + server function that has already pushed the error on the warning stack. + + @return error code given as input or stored in the context object if + a fatal error was reported before. + */ +inline +int Backup_restore_ctx::log_error(int error_code, ...) +{ + if (m_error) + return m_error; + + bool saved = push_errors(FALSE); // Do not use warning stack + + m_error= error_code; + m_remove_loc= TRUE; + + va_list args; + va_start(args,error_code); + v_report_error(backup::log_level::ERROR, error_code, args); + va_end(args); + + push_errors(saved); // Reset + + return error_code; } + /** Destroy a backup/restore context. @@ -803,6 +830,7 @@ int Backup_restore_ctx::unlock_tables() */ int Backup_restore_ctx::close() { + int error= 0; if (m_state == CLOSED) return 0; @@ -811,15 +839,10 @@ int Backup_restore_ctx::close() time_t when= my_time(0); // unlock tables if they are still locked - - // FIXME: detect errors if reported. - unlock_tables(); + unlock_tables(); // Never errors // unfreeze meta-data - - // FIXME: detect errors if reported. - // FIXME: error logging. - obs::ddl_blocker_disable(); + obs::ddl_blocker_disable(); // Never errors // restore thread options @@ -827,10 +850,11 @@ int Backup_restore_ctx::close() // close stream - if (m_stream) - // FIXME: detect errors if reported. - // FIXME: error logging. - m_stream->close(); + if (m_stream && !m_stream->close()) + { + // Note error, but complete clean-up + error= ER_BACKUP_CLOSE; + } if (m_catalog) m_catalog->save_end_time(when); // Note: no errors. @@ -850,16 +874,24 @@ int Backup_restore_ctx::close() */ if (res && my_errno != ENOENT) { - report_error(ER_CANT_DELETE_FILE, m_path, my_errno); - if (!m_error) - m_error= ER_CANT_DELETE_FILE; + error= ER_CANT_DELETE_FILE; } } - // We report completion of the operation only if no errors were detected. - - if (!m_error) - report_stop(when, TRUE); + /* We report completion of the operation only if no errors were detected, + and logger has been initialized. + */ + if (!error) + { + if (backup::Logger::m_state == backup::Logger::RUNNING) + { + report_stop(when, TRUE); + } + } + else + { + fatal_error(error); // Log error + } /* Destroy backup stream's memory allocator (this frees memory) @@ -880,7 +912,7 @@ int Backup_restore_ctx::close() pthread_mutex_unlock(&run_lock); m_state= CLOSED; - return m_error; + return error; } /** @@ -908,9 +940,7 @@ int Backup_restore_ctx::do_backup() DEBUG_SYNC(m_thd, "before_backup_meta"); - // FIXME: detect errors if reported. - // FIXME: error logging. - report_stats_pre(info); + report_stats_pre(info); // Never errors DBUG_PRINT("backup",("Writing preamble")); DEBUG_SYNC(m_thd, "backup_before_write_preamble"); @@ -925,7 +955,7 @@ int Backup_restore_ctx::do_backup() DEBUG_SYNC(m_thd, "before_backup_data"); - if (write_table_data(m_thd, info, s)) // reports errors + if (write_table_data(m_thd, info, s)) // logs errors DBUG_RETURN(send_error(*this, ER_BACKUP_BACKUP)); DBUG_PRINT("backup",("Writing summary")); @@ -936,9 +966,7 @@ int Backup_restore_ctx::do_backup() DBUG_RETURN(m_error); } - // FIXME: detect errors if reported. - // FIXME: error logging. - report_stats_post(info); + report_stats_post(info); // Never errors DBUG_PRINT("backup",("Backup done.")); DEBUG_SYNC(m_thd, "before_backup_done"); @@ -966,30 +994,38 @@ int Backup_restore_ctx::restore_triggers DBUG_ASSERT(m_catalog); - // FIXME: detect errors (when dbit==NULL). Perhaps just assert. - Image_info::Iterator *dbit= m_catalog->get_dbs(); + DBUG_ENTER("restore_triggers_and_events"); + Image_info::Obj *obj; List<Image_info::Obj> events; Image_info::Obj::describe_buf buf; - DBUG_ENTER("restore_triggers_and_events"); + Image_info::Iterator *dbit= m_catalog->get_dbs(); + if (!dbit) + { + DBUG_RETURN(fatal_error(ER_OUT_OF_RESOURCES)); + } // create all trigers and collect events in the events list while ((obj= (*dbit)++)) { - // FIXME: detect errors (when it==NULL). Perhaps just assert. - Image_info::Iterator *it= + Image_info::Iterator *it= m_catalog->get_db_objects(*static_cast<Image_info::Db*>(obj)); - + if (!it) + { + DBUG_RETURN(fatal_error(ER_OUT_OF_RESOURCES)); + } while ((obj= (*it)++)) switch (obj->type()) { case BSTREAM_IT_EVENT: DBUG_ASSERT(obj->m_obj_ptr); - // FIXME: detect errors if reported. - // FIXME: error logging. - events.push_back(obj); + if (events.push_back(obj)) + { + // Error has been reported, but not logged to backup logs + DBUG_RETURN(log_error(ER_OUT_OF_RESOURCES)); + } break; case BSTREAM_IT_TRIGGER: @@ -1051,27 +1087,24 @@ int Backup_restore_ctx::do_restore() Input_stream &s= *static_cast<Input_stream*>(m_stream); Restore_info &info= *static_cast<Restore_info*>(m_catalog); - // FIXME: detect errors if reported. - // FIXME: error logging. - report_stats_pre(info); + report_stats_pre(info); // Never errors DBUG_PRINT("restore", ("Restoring meta-data")); - // FIXME: detect errors if reported. - disable_fkey_constraints(); // reports errors + disable_fkey_constraints(); // Never errors if (read_meta_data(info, s)) { - // FIXME: detect errors. - // FIXME: error logging. - m_thd->main_da.reset_diagnostics_area(); + m_thd->main_da.reset_diagnostics_area(); // Never errors fatal_error(ER_BACKUP_READ_META); DBUG_RETURN(m_error); } - // FIXME: detect errors. - s.next_chunk(); + if (s.next_chunk() == BSTREAM_ERROR) + { + DBUG_RETURN(fatal_error(ER_BACKUP_NEXT_CHUNK)); + } DBUG_PRINT("restore",("Restoring table data")); @@ -1081,21 +1114,16 @@ int Backup_restore_ctx::do_restore() It should be fixed inside object services implementation and then the following line should be removed. */ - // FIXME: detect errors. - // FIXME: error logging. - close_thread_tables(m_thd); - // FIXME: detect errors. - // FIXME: error logging. - m_thd->main_da.reset_diagnostics_area(); + close_thread_tables(m_thd); // Never errors + m_thd->main_da.reset_diagnostics_area(); // Never errors - if (lock_tables_for_restore()) // reports errors + if (lock_tables_for_restore()) // logs errors DBUG_RETURN(m_error); // Here restore drivers are created to restore table data err= restore_table_data(m_thd, info, s); // reports errors - // FIXME: detect errors if reported. - unlock_tables(); + unlock_tables(); // Never errors if (err) DBUG_RETURN(ER_BACKUP_RESTORE); @@ -1124,12 +1152,8 @@ int Backup_restore_ctx::do_restore() It should be fixed inside object services implementation and then the following line should be removed. */ - // FIXME: detect errors. - // FIXME: error logging. - close_thread_tables(m_thd); - // FIXME: detect errors. - // FIXME: error logging. - m_thd->main_da.reset_diagnostics_area(); + close_thread_tables(m_thd); // Never errors + m_thd->main_da.reset_diagnostics_area(); // Never errors /* Report validity point time and binlog position stored in the backup image @@ -1140,9 +1164,7 @@ int Backup_restore_ctx::do_restore() if (info.flags & BSTREAM_FLAG_BINLOG) report_binlog_pos(info.binlog_pos); - // FIXME: detect errors if reported. - // FIXME: error logging. - report_stats_post(info); + report_stats_post(info); // Never errors DBUG_RETURN(0); } @@ -1541,27 +1563,25 @@ void* bcat_iterator_get(st_bstream_image case BSTREAM_IT_TABLESPACE: // table spaces { Iterator *it= info->get_tablespaces(); - - if (!it) + if (!it) { - info->m_ctx.fatal_error(ER_BACKUP_CAT_ENUM); + info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES); return NULL; } - + return it; } case BSTREAM_IT_DB: // all databases { Iterator *it= info->get_dbs(); - - if (!it) + if (!it) { - info->m_ctx.fatal_error(ER_BACKUP_CAT_ENUM); + info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES); return NULL; } - return it; + return it; } case BSTREAM_IT_PERDB: // per-db objects, except tables @@ -1580,14 +1600,8 @@ void* bcat_iterator_get(st_bstream_image case BSTREAM_IT_GLOBAL: // all global objects { Iterator *it= info->get_global(); - - if (!it) - { - info->m_ctx.fatal_error(ER_BACKUP_CAT_ENUM); - return NULL; - } - return it; + return it; // if (!it), error has been logged in get_global() } default: @@ -1676,10 +1690,9 @@ void* bcat_db_iterator_get(st_bstream_im } backup::Image_info::Iterator *it= info->get_db_objects(*db); - if (!it) { - info->m_ctx.fatal_error(ER_BACKUP_LIST_DB_TABLES); + info->m_ctx.fatal_error(ER_OUT_OF_RESOURCES); return NULL; } @@ -2019,7 +2032,11 @@ TABLE_LIST *build_table_list(const Table for( uint tno=0; tno < tables.count() ; tno++ ) { TABLE_LIST *ptr = mk_table_list(tables[tno], lock, ::current_thd->mem_root); - DBUG_ASSERT(ptr); + if (!ptr) + { + // Failed to allocate (failure has been reported) + return NULL; + } tl= link_table_list(*ptr,tl); } === modified file 'sql/backup/logger.cc' --- a/sql/backup/logger.cc 2008-09-08 11:05:26 +0000 +++ b/sql/backup/logger.cc 2008-10-02 14:35:29 +0000 @@ -49,8 +49,9 @@ int Logger::write_message(log_level::val errors.push_front(new MYSQL_ERROR(::current_thd, error_code, MYSQL_ERROR::WARN_LEVEL_ERROR, msg)); sql_print_error(out); - push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, - error_code, msg); + if (m_push_errors) + push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, + error_code, msg); DBUG_PRINT("backup_log",("[ERROR] %s", out)); if (m_state == READY || m_state == RUNNING) @@ -65,8 +66,9 @@ int Logger::write_message(log_level::val case log_level::WARNING: sql_print_warning(out); - push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, - error_code, msg); + if (m_push_errors) + push_warning_printf(current_thd, MYSQL_ERROR::WARN_LEVEL_WARN, + error_code, msg); DBUG_PRINT("backup_log",("[Warning] %s", out)); return 0; @@ -131,4 +133,20 @@ void Logger::report_stats_post(const Ima backup_log->size(info.data_size); } + +/* + Indicate if reported errors should be pushed on the warning stack. + + If @c flag is TRUE, errors will be pushed on the warning stack, otherwise + they will not. + + @returns Current setting. +*/ +bool Logger::push_errors(bool flag) +{ + bool old= m_push_errors; + m_push_errors= flag; + return old; +} + } // backup namespace === modified file 'sql/backup/logger.h' --- a/sql/backup/logger.h 2008-09-30 07:51:48 +0000 +++ b/sql/backup/logger.h 2008-10-02 14:35:29 +0000 @@ -70,6 +70,7 @@ class Logger void stop_save_errors(); void clear_saved_errors(); MYSQL_ERROR *last_saved_error(); + bool push_errors(bool); protected: @@ -84,6 +85,8 @@ class Logger List<MYSQL_ERROR> errors; ///< Used to store saved errors. bool m_save_errors; ///< Flag telling if errors should be saved. + bool m_push_errors; ///< Should errors be pushed on warning stack? + Backup_log *backup_log; ///< Backup log interface class. }; === modified file 'sql/backup/stream.cc' --- a/sql/backup/stream.cc 2008-09-11 11:14:59 +0000 +++ b/sql/backup/stream.cc 2008-10-02 14:35:29 +0000 @@ -419,7 +419,8 @@ bool Stream::test_secure_file_priv_acces */ int Stream::open() { - close(); + close(); // If close() should fail, we will still try to open + if (!test_secure_file_priv_access(m_path.c_ptr())) return ER_OPTION_PREVENTS_STATEMENT; @@ -431,13 +432,18 @@ int Stream::open() return 0; } -void Stream::close() +bool Stream::close() { + bool ret= TRUE; if (m_fd >= 0) { - my_close(m_fd, MYF(0)); + if (my_close(m_fd, MYF(0))) + { + ret= FALSE; + } m_fd= -1; } + return ret; } bool Stream::rewind() @@ -532,7 +538,7 @@ bool Output_stream::init() int Output_stream::open() { MY_STAT stat_info; - close(); + close(); // If close() should fail, we will still try to open /* Allow to write to existing named pipe */ if (my_stat(m_path.c_ptr(), &stat_info, MYF(0)) && @@ -585,17 +591,22 @@ int Output_stream::open() Close backup stream If @c destroy is TRUE, the stream object is deleted. + + @retval TRUE Operation Succeeded + @retval FALSE Operation Failed */ -void Output_stream::close() +bool Output_stream::close() { + bool ret= TRUE; if (m_fd < 0) - return; + return TRUE; + + if (bstream_close(this) == BSTREAM_ERROR) + { + // Note that close failed, and continue with lower level clean-up. + ret= FALSE; + } - /* - Note: Even if bstream_close() fails we want to do the lower level clean-up. - This is why errors from bstream_close() are ignored. - */ - bstream_close(this); #ifdef HAVE_COMPRESS if (m_with_compression) { @@ -624,7 +635,8 @@ void Output_stream::close() my_free(zbuf, MYF(0)); } #endif - Stream::close(); + ret &= Stream::close(); + return ret; } /** @@ -723,7 +735,7 @@ bool Input_stream::init() */ int Input_stream::open() { - close(); + close(); // If close() should fail, we will still try to open int ret= Stream::open(); @@ -776,17 +788,22 @@ int Input_stream::open() Close backup stream If @c destroy is TRUE, the stream object is deleted. + + @retval TRUE Operation Succeeded + @retval FALSE Operation Failed */ -void Input_stream::close() +bool Input_stream::close() { + bool ret= TRUE; if (m_fd < 0) - return; + return TRUE; + + if (bstream_close(this) == BSTREAM_ERROR) + { + // Note that close failed, and continue with lower level clean-up. + ret= FALSE; + } - /* - Note: Even if bstream_close() fails we want to do the lower level clean-up. - This is why errors from bstream_close() are ignored. - */ - bstream_close(this); #ifdef HAVE_COMPRESS if (m_with_compression) { @@ -796,7 +813,8 @@ void Input_stream::close() my_free(zbuf, (MYF(0))); } #endif - Stream::close(); + ret &= Stream::close(); + return ret; } /** === modified file 'sql/backup/stream.h' --- a/sql/backup/stream.h 2008-09-02 09:04:39 +0000 +++ b/sql/backup/stream.h 2008-10-02 14:35:29 +0000 @@ -80,7 +80,8 @@ class Stream: public fd_stream public: int open(); - virtual void close(); + virtual bool close(); + bool rewind(); /// Check if stream is opened @@ -121,8 +122,8 @@ class Output_stream: Output_stream(Logger&, ::String *, LEX_STRING, bool); - int open(); - void close(); + int open(); + bool close(); bool rewind(); private: @@ -139,8 +140,8 @@ class Input_stream: Input_stream(Logger&, ::String *, LEX_STRING); - int open(); - void close(); + int open(); + bool close(); bool rewind(); int next_chunk(); === modified file 'sql/mdl.cc' --- a/sql/mdl.cc 2008-08-13 10:36:29 +0000 +++ b/sql/mdl.cc 2008-10-02 14:35:29 +0000 @@ -300,7 +300,7 @@ void mdl_init_lock(MDL_LOCK_DATA *lock_d @note The allocated lock request will have MDL_SHARED type. - @retval 0 Error + @retval 0 Error if out of memory @retval non-0 Pointer to an object representing a lock request */ === modified file 'sql/share/errmsg.txt' --- a/sql/share/errmsg.txt 2008-09-29 18:41:59 +0000 +++ b/sql/share/errmsg.txt 2008-10-02 14:35:29 +0000 @@ -6392,3 +6392,16 @@ ER_BACKUP_LOGPATHS eng "The log names for backup_history and backup_progress must be unique." ER_BACKUP_LOGPATH_INVALID eng "The path specified for the %-.64s is invalid. ref: %-.64s" +ER_BACKUP_SEND_REPLY + eng "Failed to send reply to client after successful %-.64s operation" + nor "Mislykket sending av svar til klient etter %-.64s" + norwegian-ny "Sendinga av svar til klienten etter %-.64s" +ER_BACKUP_CLOSE + eng "Backup/Restore: Error on close of backup stream" + nor "Backup/Restore: Feil ved lukning av backup-strøm" + norwegian-ny "Backup/Restore: Feil ved lukking av backup-straum" +ER_BACKUP_BINLOG + eng "Error on accessing binlog during BACKUP" + nor "Lesing av binlog feilet under backup" + norwegian-ny "Lesing av binlog feila under backup" +
| Thread | ||
|---|---|---|
| • bzr commit into mysql-6.0-backup branch (oystein.grovlen:2700)Bug#39089 WL#4384 | Oystein.Grovlen | 2 Oct |
| • Re: bzr commit into mysql-6.0-backup branch (oystein.grovlen:2700)Bug#39089 WL#4384 | Jørgen Løland | 13 Oct |
| • Re: bzr commit into mysql-6.0-backup branch (oystein.grovlen:2700)Bug#39089 WL#4384 | Øystein Grøvlen | 14 Oct |
© 1995-2008 MySQL AB, 2008-2009 Sun Microsystems, Inc.
Page generated in 0.724 sec. using MySQL 5.1.14-beta-log