From: Date: September 10 2008 1:20am Subject: bzr push into mysql-6.0-falcon branch (cpowers:2813 to 2814) Bug#39347 Bug#39349 Bug#39354 List-Archive: http://lists.mysql.com/commits/53649 X-Bug: 39347,39349,39354 Message-Id: <20080909232033.BEB4A1DB05F0@xeno.mysql.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 2814 Christopher Powers 2008-09-09 Bug#39347 Falcon: Online add/drop unique index fails Bug#39349 Falcon: Online drop index conflicts with other index operations Bug#39354 Falcon: Rename table corrupts index mapping - Enabled HA_ADD/DROP_UNIQUE_INDEX - Don't create StorageIndexDesc objects if internal falcon index does not exist - Rebuild index mapping when table is renamed - Added validation methods to detect out-of-sync condition between server and falcon indexes modified: storage/falcon/Configuration.cpp storage/falcon/StorageTable.cpp storage/falcon/StorageTableShare.cpp storage/falcon/StorageTableShare.h storage/falcon/ha_falcon.cpp storage/falcon/ha_falcon.h 2813 Kelly Long 2008-09-09 [merge] Merged. removed: mysql-test/suite/falcon/r/falcon_select_excerpt.result mysql-test/suite/falcon/t/falcon_select_excerpt.test added: mysql-test/suite/falcon/r/falcon_bug_38304.result mysql-test/suite/falcon/r/falcon_index_datatypes.result mysql-test/suite/falcon/r/falcon_online_index.result mysql-test/suite/falcon/t/falcon_bug_38304.test mysql-test/suite/falcon/t/falcon_index_datatypes.test mysql-test/suite/falcon/t/falcon_online_index.test modified: extra/CMakeLists.txt storage/falcon/Table.cpp storage/falcon/Transaction.cpp === modified file 'storage/falcon/Configuration.cpp' --- a/storage/falcon/Configuration.cpp 2008-08-29 20:41:13 +0000 +++ b/storage/falcon/Configuration.cpp 2008-09-09 23:15:17 +0000 @@ -42,7 +42,6 @@ #include "SQLError.h" #include "Log.h" #include "IOx.h" -#include "ScanDir.h" #ifndef ULL #define ULL(a) ((uint64) a) === modified file 'storage/falcon/StorageTable.cpp' --- a/storage/falcon/StorageTable.cpp 2008-08-22 06:47:40 +0000 +++ b/storage/falcon/StorageTable.cpp 2008-09-09 23:15:17 +0000 @@ -195,7 +195,7 @@ int StorageTable::setCurrentIndex(int in } if (!(currentIndex = share->getIndex(indexId))) -{ + { clearCurrentIndex(); return StorageErrorNoIndex; } === modified file 'storage/falcon/StorageTableShare.cpp' --- a/storage/falcon/StorageTableShare.cpp 2008-09-03 22:17:54 +0000 +++ b/storage/falcon/StorageTableShare.cpp 2008-09-09 23:15:17 +0000 @@ -62,10 +62,10 @@ StorageIndexDesc::StorageIndexDesc() rawName[0] = '\0'; }; -StorageIndexDesc::StorageIndexDesc(const StorageIndexDesc *indexInfo) +StorageIndexDesc::StorageIndexDesc(const StorageIndexDesc *indexDesc) { - if (indexInfo) - *this = *indexInfo; + if (indexDesc) + *this = *indexDesc; else { id = 0; @@ -86,6 +86,35 @@ StorageIndexDesc::~StorageIndexDesc(void { } +bool StorageIndexDesc::operator==(const StorageIndexDesc &indexDesc) const +{ + if ( !(id == indexDesc.id + && unique == indexDesc.unique + && primaryKey == indexDesc.primaryKey + && numberSegments == indexDesc.numberSegments)) + return false; + + if (strcmp(indexDesc.name, name) != 0) + return false; + + /*** + const char *q = indexDesc.name; + const char *p = name; + + for (; (*p && *q); ++p, ++q) + if (*p != *q) + return false; + ***/ + + return true; +} + +bool StorageIndexDesc::operator!=(const StorageIndexDesc &indexDesc) const +{ + return !(*this == indexDesc); +} + + ////////////////////////////////////////////////////////////////////// // Construction/Destruction ////////////////////////////////////////////////////////////////////// @@ -285,11 +314,17 @@ const char* StorageTableShare::cleanupTa return buffer; } -char* StorageTableShare::createIndexName(const char *rawName, char *indexName) +char* StorageTableShare::createIndexName(const char *rawName, char *indexName, bool primary) { - char nameBuffer[indexNameSize]; - cleanupFieldName(rawName, nameBuffer, sizeof(nameBuffer)); - sprintf(indexName, "%s$%s", name.getString(), nameBuffer); + if (primary) + strcpy(indexName, table->getPrimaryKeyName().getString()); + else + { + char nameBuffer[indexNameSize]; + cleanupFieldName(rawName, nameBuffer, sizeof(nameBuffer)); + sprintf(indexName, "%s$%s", name.getString(), nameBuffer); + } + return indexName; } @@ -362,12 +397,35 @@ int StorageTableShare::dropIndex(Storage int ret = storageDatabase->dropIndex(storageConnection, table, sql); - if (!ret) - deleteIndex(indexDesc->id); + deleteIndex(indexDesc->id); return ret; } +bool StorageTableShare::validateIndex(int indexId, StorageIndexDesc *indexTarget) +{ + StorageIndexDesc *indexDesc = getIndex(indexId); + + if (!indexDesc || *indexDesc != *indexTarget) + return false; + + Index *index = table->findIndex(indexDesc->name); + + if (!index) + return false; + + if (index != indexDesc->index) + return false; + + if (indexTarget->primaryKey && index->type != PrimaryKey) + return false; + + if (index->recordsPerSegment != indexDesc->segmentRecordCounts) + return false; + + return true; +} + void StorageTableShare::deleteIndexes() { for (StorageIndexDesc *indexDesc; (indexDesc = indexes);) @@ -377,6 +435,16 @@ void StorageTableShare::deleteIndexes() } } +int StorageTableShare::numberIndexes() +{ + int indexCount = 0; + + for (StorageIndexDesc *indexDesc = indexes; indexDesc; indexDesc = indexDesc->next, indexCount++) + ; + + return indexCount; +} + int StorageTableShare::renameTable(StorageConnection *storageConnection, const char* newName) { char tableName[256]; @@ -402,24 +470,25 @@ int StorageTableShare::setIndex(const St if (!getIndex(indexInfo->id)) { - StorageIndexDesc *indexDesc = new StorageIndexDesc(indexInfo); - addIndex(indexDesc); - // Find the corresponding Falcon index - - if (indexDesc->primaryKey) - indexDesc->index = table->primaryKey; - else - { - char indexName[indexNameSize]; - sprintf(indexName, "%s$%s", name.getString(), indexDesc->name); - indexDesc->index = table->findIndex(indexName); - } + // Find the corresponding Falcon index else fail - if (indexDesc->index) - indexDesc->segmentRecordCounts = indexDesc->index->recordsPerSegment; - else - ret = StorageErrorNoIndex; + Index *index; + + if (indexInfo->primaryKey) + index = table->primaryKey; + else + index = table->findIndex(indexInfo->name); + + if (index) + { + StorageIndexDesc *indexDesc = new StorageIndexDesc(indexInfo); + indexDesc->index = index; + indexDesc->segmentRecordCounts = indexDesc->index->recordsPerSegment; + addIndex(indexDesc); + } + else + ret = StorageErrorNoIndex; } return ret; @@ -432,7 +501,10 @@ StorageIndexDesc* StorageTableShare::get for (StorageIndexDesc *indexDesc = indexes; indexDesc; indexDesc = indexDesc->next) if (indexDesc->id == indexId) + { + ASSERT(indexDesc->index != NULL); return indexDesc; + } return NULL; } @@ -453,21 +525,6 @@ StorageIndexDesc* StorageTableShare::get return index; } -StorageIndexDesc* StorageTableShare::getIndex(const char *name) -{ - if (!indexes) - return NULL; - - Sync sync(syncIndexes, "StorageTableShare::getIndex(name)"); - sync.lock(Shared); - - for (StorageIndexDesc *indexDesc = indexes; indexDesc; indexDesc = indexDesc->next) - if (indexDesc->name == name) - return indexDesc; - - return NULL; -} - int StorageTableShare::getIndexId(const char* schemaName, const char* indexName) { if (!indexes) === modified file 'storage/falcon/StorageTableShare.h' --- a/storage/falcon/StorageTableShare.h 2008-08-22 06:47:40 +0000 +++ b/storage/falcon/StorageTableShare.h 2008-09-09 23:15:17 +0000 @@ -53,8 +53,10 @@ class StorageIndexDesc { public: StorageIndexDesc(); - StorageIndexDesc(const StorageIndexDesc *indexInfo); + StorageIndexDesc(const StorageIndexDesc *indexDesc); virtual ~StorageIndexDesc(void); + bool operator==(const StorageIndexDesc &indexDesc) const; + bool operator!=(const StorageIndexDesc &indexDesc) const; int id; int unique; @@ -113,7 +115,9 @@ public: virtual void unlockIndexes(void); virtual int createIndex(StorageConnection *storageConnection, StorageIndexDesc *indexDesc, const char *sql); virtual int dropIndex(StorageConnection *storageConnection, StorageIndexDesc *indexDesc, const char *sql); + virtual bool validateIndex(int indexId, StorageIndexDesc *indexTarget); virtual void deleteIndexes(); + virtual int numberIndexes(); virtual int renameTable(StorageConnection *storageConnection, const char* newName); virtual INT64 getSequenceValue(int delta); virtual int setSequenceValue(INT64 value); @@ -129,7 +133,6 @@ public: void clearIndex(StorageIndexDesc *indexDesc); StorageIndexDesc* getIndex(int indexId); StorageIndexDesc* getIndex(int indexId, StorageIndexDesc *indexDesc); - StorageIndexDesc* getIndex(const char *name); int getIndexId(const char* schemaName, const char* indexName); int create(StorageConnection *storageConnection, const char* sql, int64 autoIncrementValue); int upgrade(StorageConnection *storageConnection, const char* sql, int64 autoIncrementValue); @@ -148,7 +151,7 @@ public: static const char* getDefaultRoot(void); static const char* cleanupTableName(const char* name, char* buffer, int bufferLength, char *schema, int schemaLength); - char* createIndexName(const char *rawName, char *indexName); + char* createIndexName(const char *rawName, char *indexName, bool primary = false); JString name; JString schemaName; === modified file 'storage/falcon/ha_falcon.cpp' --- a/storage/falcon/ha_falcon.cpp 2008-09-05 16:26:12 +0000 +++ b/storage/falcon/ha_falcon.cpp 2008-09-09 23:15:17 +0000 @@ -518,11 +518,11 @@ int StorageInterface::open(const char *n thr_lock_data_init((THR_LOCK *)storageShare->impure, &lockData, NULL); - ret = setIndexes(); - if (table) mapFields(table); + setIndexes(table); + if (ret) DBUG_RETURN(error(ret)); @@ -850,6 +850,8 @@ int StorageInterface::create(const char } mapFields(form); + + setIndexes(table); DBUG_RETURN(0); } @@ -868,12 +870,9 @@ int StorageInterface::createIndex(const StorageIndexDesc indexDesc; getKeyDesc(table, indexId, &indexDesc); - char indexName[indexNameSize]; - storageShare->createIndexName(indexDesc.name, indexName); - CmdGen gen; const char *unique = (key->flags & HA_NOSAME) ? "unique " : ""; - gen.gen("create %sindex \"%s\" on %s.\"%s\" ", unique, indexName, schemaName, tableName); + gen.gen("create %sindex \"%s\" on %s.\"%s\" ", unique, indexDesc.name, schemaName, tableName); genKeyFields(key, &gen); const char *sql = gen.getString(); @@ -885,11 +884,8 @@ int StorageInterface::dropIndex(const ch StorageIndexDesc indexDesc; getKeyDesc(table, indexId, &indexDesc); - char indexName[indexNameSize]; - storageShare->createIndexName(indexDesc.name, indexName); - CmdGen gen; - gen.gen("drop index %s.\"%s\"", schemaName, indexName); + gen.gen("drop index %s.\"%s\"", schemaName, indexDesc.name); const char *sql = gen.getString(); return storageTable->dropIndex(&indexDesc, sql); @@ -1394,8 +1390,9 @@ int StorageInterface::index_init(uint id nextRecord = 0; haveStartKey = false; haveEndKey = false; + int ret = 0; - int ret = storageTable->setCurrentIndex(idx); + ret = storageTable->setCurrentIndex(idx); if (ret) { @@ -1403,6 +1400,8 @@ int StorageInterface::index_init(uint id ret = storageTable->setCurrentIndex(idx); } + // validateIndexes(table); + if (ret) DBUG_RETURN(error(ret)); @@ -1465,22 +1464,18 @@ void StorageInterface::getKeyDesc(TABLE { KEY *keyInfo = table->key_info + indexId; int numberKeys = keyInfo->key_parts; - char nameBuffer[indexNameSize]; + + indexDesc->id = indexId; + indexDesc->numberSegments = numberKeys; + indexDesc->unique = (keyInfo->flags & HA_NOSAME); + indexDesc->primaryKey = (table->s->primary_key == (uint)indexId); // Clean up the index name for internal use strncpy(indexDesc->rawName, (const char*)keyInfo->name, MIN(indexNameSize, (int)strlen(keyInfo->name)+1)); - storageShare->cleanupFieldName(indexDesc->rawName, nameBuffer, sizeof(nameBuffer)); indexDesc->rawName[indexNameSize-1] = '\0'; + storageShare->createIndexName(indexDesc->rawName, indexDesc->name, indexDesc->primaryKey); - strncpy(indexDesc->name, (const char*)nameBuffer, MIN(indexNameSize, (int)strlen(nameBuffer)+1)); - indexDesc->name[indexNameSize-1] = '\0'; - - indexDesc->id = indexId; - indexDesc->numberSegments = numberKeys; - indexDesc->unique = (keyInfo->flags & HA_NOSAME); - indexDesc->primaryKey = (table->s->primary_key == (uint)indexId); - for (int n = 0; n < numberKeys; ++n) { StorageSegment *segment = indexDesc->segments + n; @@ -1519,8 +1514,17 @@ int StorageInterface::rename_table(const if (ret) DBUG_RETURN(error(ret)); + storageTable->clearCurrentIndex(); + storageShare->lockIndexes(true); + storageShare->lock(true); + ret = storageShare->renameTable(storageConnection, to); + remapIndexes(table); + + storageShare->unlock(); + storageShare->unlockIndexes(); + if (ret) DBUG_RETURN(error(ret)); @@ -1541,6 +1545,7 @@ int StorageInterface::read_range_first(c storageTable->clearIndexBounds(); haveStartKey = false; haveEndKey = false; + int ret = 0; if (start_key && !storageTable->isKeyNull((const unsigned char*) start_key->key, start_key->length)) { @@ -1563,7 +1568,7 @@ int StorageInterface::read_range_first(c if (end_key->flag == HA_READ_BEFORE_KEY) storageTable->setReadBeforeKey(); - int ret = storageTable->setIndexBound((const unsigned char*) end_key->key, + ret = storageTable->setIndexBound((const unsigned char*) end_key->key, end_key->length, UpperBound); if (ret) DBUG_RETURN(error(ret)); @@ -2109,10 +2114,9 @@ int StorageInterface::check_if_supported DBUG_ENTER("StorageInterface::check_if_supported_alter"); tempTable = (create_info->options & HA_LEX_CREATE_TMP_TABLE) ? true : false; HA_ALTER_FLAGS supported; - supported = supported | HA_ADD_INDEX | HA_DROP_INDEX; + supported = supported | HA_ADD_INDEX | HA_DROP_INDEX | HA_ADD_UNIQUE_INDEX | HA_DROP_UNIQUE_INDEX; /** - | HA_ADD_COLUMN | HA_ADD_UNIQUE_INDEX | HA_DROP_UNIQUE_INDEX - | HA_COLUMN_STORAGE | HA_COLUMN_FORMAT; + | HA_ADD_COLUMN | HA_COLUMN_STORAGE | HA_COLUMN_FORMAT; **/ HA_ALTER_FLAGS notSupported = ~(supported); @@ -2153,7 +2157,7 @@ int StorageInterface::check_if_supported // 1. Check for supported ALTER combinations // 2. Can error message be improved for non-null columns? - if (alter_flags->is_set(HA_ADD_INDEX)) + if (alter_flags->is_set(HA_ADD_INDEX) || alter_flags->is_set(HA_ADD_UNIQUE_INDEX)) { for (unsigned int n = 0; n < altered_table->s->keys; n++) { @@ -2185,7 +2189,7 @@ int StorageInterface::check_if_supported } } - if (alter_flags->is_set(HA_DROP_INDEX)) + if (alter_flags->is_set(HA_DROP_INDEX) || alter_flags->is_set(HA_DROP_UNIQUE_INDEX)) { } @@ -2200,10 +2204,10 @@ int StorageInterface::alter_table_phase1 if (alter_flags->is_set(HA_ADD_COLUMN)) ret = addColumn(thd, altered_table, create_info, alter_info, alter_flags); - if (alter_flags->is_set(HA_ADD_INDEX) && !ret) + if ((alter_flags->is_set(HA_ADD_INDEX) || alter_flags->is_set(HA_ADD_UNIQUE_INDEX)) && !ret) ret = addIndex(thd, altered_table, create_info, alter_info, alter_flags); - if (alter_flags->is_set(HA_DROP_INDEX) && !ret) + if ((alter_flags->is_set(HA_DROP_INDEX) || alter_flags->is_set(HA_DROP_UNIQUE_INDEX)) && !ret) ret = dropIndex(thd, altered_table, create_info, alter_info, alter_flags); DBUG_RETURN(ret); @@ -2386,7 +2390,7 @@ int StorageInterface::setIndex(TABLE *ta return storageTable->setIndex(&indexDesc); } -int StorageInterface::setIndexes(void) +int StorageInterface::setIndexes(TABLE *table) { int ret = 0; @@ -2394,14 +2398,14 @@ int StorageInterface::setIndexes(void) return ret; storageShare->lockIndexes(true); + storageShare->lock(true); - if (!storageShare->haveIndexes(table->s->keys)) - for (uint n = 0; n < table->s->keys; ++n) - if ((ret = setIndex(table, n))) - break; + ret = remapIndexes(table); + // validateIndexes(table, true); + storageShare->unlock(); storageShare->unlockIndexes(); - + return ret; } @@ -2409,11 +2413,11 @@ int StorageInterface::remapIndexes(TABLE { int ret = 0; + storageShare->deleteIndexes(); + if (!table) return ret; - storageShare->deleteIndexes(); - for (uint n = 0; n < table->s->keys; ++n) if ((ret = setIndex(table, n))) break; @@ -2421,6 +2425,32 @@ int StorageInterface::remapIndexes(TABLE return ret; } +bool StorageInterface::validateIndexes(TABLE *table, bool exclusiveLock) +{ + bool ret = true; + + if (!table) + return false; + + storageShare->lockIndexes(exclusiveLock); + + for (uint n = 0; (n < table->s->keys) && ret; ++n) + { + StorageIndexDesc indexDesc; + getKeyDesc(table, n, &indexDesc); + + if (!storageShare->validateIndex(n, &indexDesc)) + ret = false; + } + + if (ret && (table->s->keys != (uint)storageShare->numberIndexes())) + ret = false; + + storageShare->unlockIndexes(); + + return ret; +} + int StorageInterface::genTable(TABLE* table, CmdGen* gen) { const char *tableName = storageTable->getName(); === modified file 'storage/falcon/ha_falcon.h' --- a/storage/falcon/ha_falcon.h 2008-09-03 22:17:54 +0000 +++ b/storage/falcon/ha_falcon.h 2008-09-09 23:15:17 +0000 @@ -122,8 +122,9 @@ public: int error(int storageError); void freeActiveBlobs(void); int setIndex(TABLE *table, int indexId); - int setIndexes(void); + int setIndexes(TABLE *table); int remapIndexes(TABLE *table); + bool validateIndexes(TABLE *table, bool exclusiveLock = false); int genTable(TABLE* table, CmdGen* gen); int genType(Field *field, CmdGen *gen); void genKeyFields(KEY *key, CmdGen *gen);