From: Date: September 4 2008 12:50pm Subject: bzr commit into mysql-6.0-falcon branch (vvaintroub:2808) Bug#39138 List-Archive: http://lists.mysql.com/commits/53247 X-Bug: 39138 Message-Id: <200809041050.m84AoCGR005024@mail.mysql.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit #At file:///G:/bzr/bug39138/ 2808 Vladislav Vaintroub 2008-09-04 Bug#39138 : Falcon total database deletion after CREATE + DROP TABLESPACE + recovery Problem: Falcon misinterpretins some errors from openDatabase(), assumes database does not exist and tries to create a new database. During creation, it overwrites already existing files (by passing O_CREAT|O_TRUNC to open() on Posix, and CREATE_ALWAYS to CreateFile on Windows). This is corrected in this patch - open() is now called with O_CREAT|O_EXCL and CreateFile() with CREATE_NEW. This will make Falcon never overwrite existing files. - To ensure the proper cleanup of files created during create database attempt, "create" status is tracked. On error, createDatabase sets global deleteFilesOnExit flag - this causes new files to be deleted in destructors of IO and SerialLogFile. But there is no cleanup if process terminates with exception/signal. A more elegant solution for cleanup is possible on file systems that support user-mode transactions (like recent NTFS), by making the whole createDatabase/openDatabase a single transaction with commit() at the end. Unfortunately this solution is not portable yet, not even on downlevel Windows ,so it is not implemented. - We also ensure with ASSERT, that createDatabase never opens existing files, but always creates new ones. - We do not attempt to create a database after failing recovery anymore. modified: storage/falcon/Database.cpp storage/falcon/IO.cpp storage/falcon/IOx.h storage/falcon/SQLException.h storage/falcon/SerialLogFile.cpp storage/falcon/SerialLogFile.h storage/falcon/StorageHandler.cpp storage/falcon/StorageHandler.h storage/falcon/ha_falcon.cpp per-file messages: storage/falcon/Database.cpp -If createDatabase fails, set deleteFilesOnExit flag, instead of manually deleting the master tablespace- there can be more files to delete than just falcon_master.fts - do not try opening serial log error in current directory if open fails on some reason -if recovery fails, throw new RECOVERY_ERROR exception. Good for diagnostics storage/falcon/IO.cpp - Correct open flags for createFile(). It must be O_CREAT|O_EXCL not O_CREAT|O_TRUNC - in later case the file contents of existing file is destroyed and this is not what we want to happen to database content. Basically, never ever overwrite an existing file on createFile(). - Track "created" status for the files. It is used during cleanup of the failing create database. If file was created and create database fails, the file will be deleted during IO destructor. - assert, that we do not open existing files on create database (we should always create them) storage/falcon/IOx.h New variables deleteFilesOnExit - indicates failed create database attempt and used to cleanup created files. inCreateDatabase - for assert that we never try to open already existing files during "create database" storage/falcon/SQLException.h New error code RECOVERY_ERROR storage/falcon/SerialLogFile.cpp - Correct open flags for open with "create". It must be O_CREAT|O_EXCL not O_CREAT|O_TRUNC - in later case the file contents of existing file is destroyed and this is not what we want. Basically, never ever overwrite an existing file. On Windows, use CREATE_NEW when creating a new file and OPEN_EXISTING when opening a file, instead of CREATE_ALWAYS or OPEN_ALWAYS. - Track "created" status for the files. It is used during cleanup of the failing create database. If file was created and create database fails, the file will be deleted during SerialLogFile destructor. storage/falcon/StorageHandler.cpp -new function freeFalconStorageHandler -instead of using 2 static variables storageHandler in both StorageHandler.cpp and ha_falcon.cpp, use single global variable. - refactor code to create database from exception handler in initialize() into dedicated function. On any error, set global deleteFilesOnExit flags for proper cleanup. storage/falcon/StorageHandler.h new function createDatabase storage/falcon/ha_falcon.cpp - do a proper cleanup, if init() fails ( i.e call falcon_deinit()) when exception is caught - when deinitializing falcon handler, free storageHandler memory - simplify code to for shutdown()/panic() - as it does the same as deinit(), just call deinit(). === modified file 'storage/falcon/Database.cpp' --- a/storage/falcon/Database.cpp 2008-08-11 13:22:53 +0000 +++ b/storage/falcon/Database.cpp 2008-09-04 10:49:57 +0000 @@ -679,11 +679,10 @@ void Database::createDatabase(const char } catch (...) { - dbb->closeFile(); - dbb->deleteFile(); - + deleteFilesOnExit = true; throw; } + } void Database::openDatabase(const char * filename) @@ -705,34 +704,18 @@ void Database::openDatabase(const char * { if (dbb->logLength) serialLog->copyClone(dbb->logRoot, dbb->logOffset, dbb->logLength); + serialLog->open(dbb->logRoot, false); + if (dbb->tableSpaceSectionId) + tableSpaceManager->bootstrap(dbb->tableSpaceSectionId); - try + try { - serialLog->open(dbb->logRoot, false); + serialLog->recover(); } - catch (SQLException&) + catch(SQLError &e) { - const char *p = strrchr(filename, '.'); - JString logRoot = (p) ? JString(filename, (int) (p - filename)) : name; - bool failed = true; - - try - { - serialLog->open(logRoot, false); - failed = false; - } - catch (...) - { - } - - if (failed) - throw; + throw SQLError(RECOVERY_ERROR, "Recovery failed: %s",e.getText()); } - - if (dbb->tableSpaceSectionId) - tableSpaceManager->bootstrap(dbb->tableSpaceSectionId); - - serialLog->recover(); tableSpaceManager->postRecovery(); serialLog->start(); } === modified file 'storage/falcon/IO.cpp' --- a/storage/falcon/IO.cpp 2008-08-29 20:41:13 +0000 +++ b/storage/falcon/IO.cpp 2008-09-04 10:49:57 +0000 @@ -114,6 +114,8 @@ static int simulateDiskFull = SIMULATE_D static FILE *traceFile; static char baseDir[PATH_MAX+1]={0}; +bool deleteFilesOnExit = false; +bool inCreateDatabase = false; #ifdef _DEBUG #undef THIS_FILE @@ -133,6 +135,7 @@ IO::IO() dbb = NULL; forceFsync = true; fatalError = false; + created = false; memset(writeTypes, 0, sizeof(writeTypes)); syncObject.setName("IO::syncObject"); } @@ -141,6 +144,8 @@ IO::~IO() { traceClose(); closeFile(); + if(created && deleteFilesOnExit) + deleteFile(); } static bool isAbsolutePath(const char *name) @@ -178,6 +183,8 @@ static JString getPath(const char *filen bool IO::openFile(const char * name, bool readOnly) { + ASSERT(!inCreateDatabase); + fileName = getPath(name); for (int attempt = 0; attempt < 3; ++attempt) @@ -198,6 +205,7 @@ bool IO::openFile(const char * name, boo } isReadOnly = readOnly; + created = false; #ifndef _WIN32 signal (SIGXFSZ, SIG_IGN); @@ -227,8 +235,8 @@ bool IO::createFile(const char *name) for (int attempt = 0; attempt < 3; ++attempt) { - fileId = ::open (fileName, - getWriteMode(attempt) | O_CREAT | O_RDWR | O_RANDOM | O_TRUNC | O_BINARY, + fileId = ::open (fileName.getString(), + getWriteMode(attempt) | O_CREAT | O_RDWR | O_RANDOM | O_EXCL | O_BINARY, S_IREAD | S_IWRITE | S_IRGRP | S_IWGRP); if (fileId >= 0) @@ -252,7 +260,7 @@ bool IO::createFile(const char *name) fcntl(fileId, F_SETLK, &lock); #endif #endif - + created = true; return fileId != -1; } === modified file 'storage/falcon/IOx.h' --- a/storage/falcon/IOx.h 2008-08-29 20:41:13 +0000 +++ b/storage/falcon/IOx.h 2008-09-04 10:49:57 +0000 @@ -107,9 +107,11 @@ public: bool fatalError; bool isReadOnly; bool forceFsync; + bool created; //private: Dbb *dbb; // this is a crock and should be phased out }; - +extern bool deleteFilesOnExit; +extern bool inCreateDatabase; #endif // !defined(AFX_IO_H__6A019C19_A340_11D2_AB5A_0000C01D2301__INCLUDED_) === modified file 'storage/falcon/SQLException.h' --- a/storage/falcon/SQLException.h 2008-04-22 09:19:03 +0000 +++ b/storage/falcon/SQLException.h 2008-09-04 10:49:57 +0000 @@ -69,7 +69,8 @@ enum SqlCode { TABLESPACE_NOT_EXIST_ERROR = -35, DEVICE_FULL = -36, FILE_ACCESS_ERROR = -37, - TABLESPACE_DATAFILE_EXIST_ERROR = -38 + TABLESPACE_DATAFILE_EXIST_ERROR = -38, + RECOVERY_ERROR = -39 }; class DllExport SQLException { === modified file 'storage/falcon/SerialLogFile.cpp' --- a/storage/falcon/SerialLogFile.cpp 2008-07-15 18:57:27 +0000 +++ b/storage/falcon/SerialLogFile.cpp 2008-09-04 10:49:57 +0000 @@ -76,16 +76,22 @@ SerialLogFile::SerialLogFile(Database *d highWater = 0; writePoint = 0; forceFsync = false; + created = false; sectorSize = database->serialLogBlockSize; } SerialLogFile::~SerialLogFile() { close(); + if(created && deleteFilesOnExit) + unlink(fileName); } void SerialLogFile::open(JString filename, bool create) { + if(!create) + ASSERT(!inCreateDatabase); + #ifdef _WIN32 handle = 0; char pathName[1024]; @@ -96,7 +102,7 @@ void SerialLogFile::open(JString filenam GENERIC_READ | GENERIC_WRITE, 0, // share mode NULL, // security attributes - (create) ? CREATE_ALWAYS : OPEN_ALWAYS, + (create) ? CREATE_NEW : OPEN_EXISTING, FILE_FLAG_NO_BUFFERING | FILE_FLAG_RANDOM_ACCESS | FILE_FLAG_WRITE_THROUGH, 0); @@ -123,7 +129,7 @@ void SerialLogFile::open(JString filenam for (int attempt = 0; attempt < 3; ++attempt) { if (create) - handle = ::open(filename, IO::getWriteMode(attempt) | O_RDWR | O_BINARY | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP); + handle = ::open(filename, IO::getWriteMode(attempt) | O_RDWR | O_BINARY | O_CREAT|O_EXCL, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP); else handle = ::open(filename, IO::getWriteMode(attempt) | O_RDWR | O_BINARY); @@ -146,7 +152,10 @@ void SerialLogFile::open(JString filenam #endif if (create) + { + created = true; zap(); + } } void SerialLogFile::close() === modified file 'storage/falcon/SerialLogFile.h' --- a/storage/falcon/SerialLogFile.h 2008-06-23 10:22:43 +0000 +++ b/storage/falcon/SerialLogFile.h 2008-09-04 10:49:57 +0000 @@ -51,6 +51,7 @@ public: Mutex syncObject; Database *database; bool forceFsync; + bool created; #ifdef _WIN32 void *handle; === modified file 'storage/falcon/StorageHandler.cpp' --- a/storage/falcon/StorageHandler.cpp 2008-08-14 11:24:18 +0000 +++ b/storage/falcon/StorageHandler.cpp 2008-09-04 10:49:57 +0000 @@ -81,7 +81,7 @@ static const char *falconSchema [] = { class Server; extern Server* startServer(int port, const char *configFile); -static StorageHandler *storageHandler; +StorageHandler *storageHandler; static char charTable[256]; static int init(); @@ -113,6 +113,11 @@ StorageHandler* getFalconStorageHandler( return storageHandler; } +void freeFalconStorageHandler(void) +{ + delete storageHandler; + storageHandler = 0; +} void StorageHandler::setDataDirectory(const char *directory) { IO::setBaseDirectory(directory); @@ -131,6 +136,8 @@ StorageHandler::StorageHandler(int lockS syncObject.setName("StorageHandler::syncObject"); hashSyncObject.setName("StorageHandler::hashSyncObject"); dictionarySyncObject.setName("StorageHandler::dictionarySyncObject"); + inCreateDatabase=false; + deleteFilesOnExit=false; } StorageHandler::~StorageHandler(void) @@ -982,30 +989,49 @@ void StorageHandler::initialize(void) } catch (SQLException &e) { - // No point in creating a database if we got memory error. - // On FILE_ACCESS_ERROR, an external application can temporarily lock the file. - // In this both cases, trying to create database in this case could eventually - // lead to "recreate" and data loss. - int err = e.getSqlcode(); - - if (err == OUT_OF_MEMORY_ERROR || err == FILE_ACCESS_ERROR || err == VERSION_ERROR) + + // If got one of following errors, just rethrow. No point in + // trying to create database. + if (err == OUT_OF_MEMORY_ERROR || err == FILE_ACCESS_ERROR || + err == VERSION_ERROR || err == RECOVERY_ERROR) throw; - defaultDatabase->createDatabase(); - IO::deleteFile(FALCON_USER); - IO::deleteFile(FALCON_TEMPORARY); - dictionaryConnection = defaultDatabase->getOpenConnection(); - Statement *statement = dictionaryConnection->createStatement(); - JString createTableSpace = genCreateTableSpace(DEFAULT_TABLESPACE, FALCON_USER); - statement->executeUpdate(createTableSpace); - - for (const char **ddl = falconSchema; *ddl; ++ddl) - statement->executeUpdate(*ddl); - - statement->close(); - dictionaryConnection->commit(); + try + { + createDatabase(); + } + catch(SQLException &e2) + { + deleteFilesOnExit = true; // Cleanup created files + + throw SQLError(e2.getSqlcode(), + "create database failed : %s \n" \ + "prior open database failure was: %s", + e2.getText(),e.getText()); + } + catch(...) + { + deleteFilesOnExit = true; + } } +} + +void StorageHandler::createDatabase(void) +{ + // Check all files are properly opened with O_CREAT + inCreateDatabase = true; + defaultDatabase->createDatabase(); + dictionaryConnection = defaultDatabase->getOpenConnection(); + Statement *statement = dictionaryConnection->createStatement(); + JString createTableSpace = genCreateTableSpace(DEFAULT_TABLESPACE, FALCON_USER); + statement->executeUpdate(createTableSpace); + + for (const char **ddl = falconSchema; *ddl; ++ddl) + statement->executeUpdate(*ddl); + statement->close(); + dictionaryConnection->commit(); + inCreateDatabase = false; } void StorageHandler::dropTempTables(void) === modified file 'storage/falcon/StorageHandler.h' --- a/storage/falcon/StorageHandler.h 2008-08-14 11:24:18 +0000 +++ b/storage/falcon/StorageHandler.h 2008-09-04 10:49:57 +0000 @@ -53,6 +53,7 @@ class THD; extern "C" { StorageHandler* getFalconStorageHandler(int lockSize); +void freeFalconStorageHandler(void); } static const int databaseHashSize = 101; @@ -126,6 +127,7 @@ public: int closeConnections(THD* thd); int dropDatabase(const char* path); void initialize(void); + void createDatabase(void); void dropTempTables(void); void cleanFileName(const char* pathname, char* filename, int filenameLength); JString genCreateTableSpace(const char* tableSpaceName, const char* filename, const char* comment = NULL); === modified file 'storage/falcon/ha_falcon.cpp' --- a/storage/falcon/ha_falcon.cpp 2008-08-29 20:41:13 +0000 +++ b/storage/falcon/ha_falcon.cpp 2008-09-04 10:49:57 +0000 @@ -72,7 +72,7 @@ static const char *falcon_extensions[] = NullS }; -static StorageHandler *storageHandler; +extern StorageHandler *storageHandler; #define PARAMETER_UINT(_name, _text, _min, _default, _max, _flags, _update_function) \ uint falcon_##_name; @@ -164,8 +164,7 @@ int StorageInterface::falcon_init(void * StorageHandler::setDataDirectory(mysql_real_data_home); - if (!storageHandler) - storageHandler = getFalconStorageHandler(sizeof(THR_LOCK)); + storageHandler = getFalconStorageHandler(sizeof(THR_LOCK)); falcon_hton->state = SHOW_OPTION_YES; falcon_hton->db_type = DB_TYPE_FALCON; @@ -216,13 +215,12 @@ int StorageInterface::falcon_init(void * } catch(SQLException &e) { - sql_print_error("Falcon: Exception '%s' during initialization", - e.getText()); + sql_print_error("Falcon: %s", e.getText()); error = true; } catch(...) { - sql_print_error(" Falcon: General exception in initialization"); + sql_print_error("Falcon: General exception in initialization"); error = true; } @@ -230,8 +228,7 @@ int StorageInterface::falcon_init(void * if (error) { // Cleanup after error - delete storageHandler; - storageHandler = 0; + falcon_deinit(0); DBUG_RETURN(1); } @@ -242,8 +239,10 @@ int StorageInterface::falcon_init(void * int StorageInterface::falcon_deinit(void *p) { if(storageHandler) + { storageHandler->shutdownHandler(); - + freeFalconStorageHandler(); + } return 0; } @@ -2010,16 +2009,12 @@ void StorageInterface::freeActiveBlobs(v void StorageInterface::shutdown(handlerton *htons) { - if(storageHandler) - storageHandler->shutdownHandler(); + falcon_deinit(0); } int StorageInterface::panic(handlerton* hton, ha_panic_function flag) { - if(storageHandler) - storageHandler->shutdownHandler(); - - return 0; + return falcon_deinit(0); } int StorageInterface::closeConnection(handlerton *hton, THD *thd)