From: Date: February 28 2008 7:19pm Subject: bk commit into 6.0 tree (klewis:1.2580) BUG#34351 List-Archive: http://lists.mysql.com/commits/43169 X-Bug: 34351 Message-Id: <200802281820.m1SIK7QD029813@mail.mysql.com> Below is the list of changes that have just been committed into a local 6.0 repository of klewis. When klewis 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, 2008-02-28 12:19:19-06:00, klewis@klewis-mysql. +5 -0 Bug#34351 - A new function, Transaction::needToUpdate, is used to determine if Table::fetchForUpdate() should attempt to lock the record. This function looks for ANY visible record version. It is able to avoid a lock attempt for ConsistentRead transactions that start before a transaction that inserts a record. It also avoids locks then the committed version fo the record is deleted. Also, this changeset delets the Table::syncUpdate object which had no effect. In the process, Table::checkUniqueIndexes and its two subfunctions are redesigned to loop when a wait occurs instead of looping in the caller. storage/falcon/StorageDatabase.cpp@stripped, 2008-02-28 12:18:23-06:00, klewis@klewis-mysql. +3 -5 StorageDatabase::nextRow() is used to sequentially traverse the table by record number. Previously, it would increment the record number being searched on by only 1 when there was a gap in the record slot due to recently deleted records. This is not necessary because Table::fetchNext skips these gaps and returns the next possible candidate. In order to avoid calling Table::fetchNext multiple times for each record number in a gap of deleted record numbers, it needs to increment based on the record number of the previous candidate. storage/falcon/Table.cpp@stripped, 2008-02-28 12:18:32-06:00, klewis@klewis-mysql. +73 -83 I noticed once while debugging, that the Table::records pointer was NULL in the table destructor. It is not very likely, but we might as well protect against deleting a null pointer. While investigating bug #33184, I discovered that the Table::syncUpdate object was not really being used because it was only locked in shared mode. It was intended to make checkUniqueIndexes atomic so that concurrent threads do not insert the same unuique key at the same time. That may be evidenced by Bug #33498. When Table::syncUpdate was esed with exclusive locks it caused a performance problem. So the problem it was intended fix must be prevented another way. Table::syncUpdate is deleted here. In the process, checkUniqueIndexes and its two subfunctions are redesigned to loop when a wait occurs instead of looping in the caller. Bug#34351 - A new function, Transaction::needToUpdate, is used to determine if Table::fetchForUpdate() should attempt to lock the record. storage/falcon/Table.h@stripped, 2008-02-28 12:18:40-06:00, klewis@klewis-mysql. +3 -7 Table::checkUniqueIndexes and its two subfunctions are redesigned to loop when a wait occurs instead of looping in the caller. They now return true if a wait occurs, false if no duplicate was found, and they throw an exception on error storage/falcon/Transaction.cpp@stripped, 2008-02-28 12:18:47-06:00, klewis@klewis-mysql. +30 -2 Bug#34351 - A new function, Transaction::needToUpdate, is used to determine if Table::fetchForUpdate() should attempt to lock the record. This function looks for ANY visible record version. It is able to avoid a lock attempt for ConsistentRead transactions that start before a transaction that inserts a record. It also avoids locks then the committed version fo the record is deleted. storage/falcon/Transaction.h@stripped, 2008-02-28 12:18:52-06:00, klewis@klewis-mysql. +1 -0 Bug#34351 - A new function, Transaction::needToUpdate, is used to determine if Table::fetchForUpdate() should attempt to lock the record. diff -Nrup a/storage/falcon/StorageDatabase.cpp b/storage/falcon/StorageDatabase.cpp --- a/storage/falcon/StorageDatabase.cpp 2008-02-20 15:16:03 -06:00 +++ b/storage/falcon/StorageDatabase.cpp 2008-02-28 12:18:23 -06:00 @@ -233,16 +233,13 @@ int StorageDatabase::nextRow(StorageTabl Table *table = storageTable->share->table; Transaction *transaction = connection->getTransaction(); Record *candidate; - Record *record; + Record *record = NULL; try { - for (;; ++recordNumber) + for (;;) { - record = NULL; - candidate = NULL; candidate = table->fetchNext(recordNumber); - if (!candidate) return StorageErrorRecordNotFound; @@ -255,6 +252,7 @@ int StorageDatabase::nextRow(StorageTabl if (!lockForUpdate) candidate->release(); + recordNumber = candidate->recordNumber + 1; continue; } diff -Nrup a/storage/falcon/Table.cpp b/storage/falcon/Table.cpp --- a/storage/falcon/Table.cpp 2008-02-28 09:27:36 -06:00 +++ b/storage/falcon/Table.cpp 2008-02-28 12:18:32 -06:00 @@ -143,7 +143,8 @@ Table::~Table() delete view; delete backloggedRecords; - delete records; + if (records) + delete records; if (recordBitmap) recordBitmap->release(); @@ -342,9 +343,7 @@ void Table::insert(Transaction *transact if (indexes) { - do - sync.lock(ATOMIC_UPDATE); - while (!checkUniqueIndexes(transaction, record, &sync)); + checkUniqueIndexes(transaction, record); FOR_INDEXES(index, this); index->insert(record, transaction); @@ -358,9 +357,6 @@ void Table::insert(Transaction *transact insert(record, NULL, recordNumber); inserted = true; - if (indexes) - sync.unlock(); - updateInversion(record, transaction); fireTriggers(transaction, PostInsert, NULL, record); record->release(); @@ -1178,13 +1174,9 @@ void Table::update(Transaction * transac // Make insert/update atomic, then check for unique index duplicats - Sync sync(&syncUpdate, "Table::update"); - if (indexes) { - do - sync.lock(ATOMIC_UPDATE); - while (!checkUniqueIndexes(transaction, record, &sync)); + checkUniqueIndexes(transaction, record); FOR_INDEXES(index, this); index->update(oldRecord, record, transaction); @@ -1196,9 +1188,6 @@ void Table::update(Transaction * transac transaction->addRecord(record); updated = true; - if (indexes) - sync.unlock(); - updateInversion(record, transaction); fireTriggers(transaction, PostUpdate, oldRecord, record); @@ -1839,7 +1828,7 @@ bool Table::insert(Record * record, Reco if (record) record->release(); - return false; + return false; } void Table::expungeRecordVersions(RecordVersion *record, RecordScavenge *recordScavenge) @@ -2341,37 +2330,46 @@ bool Table::isDuplicate(Index *index, Re @brief Determine if the record we intend to write will have a duplicate conflict with any pending or visible records. @details For each index, call checkUniqueIndex. - Return true if the search succeeded by not finding a duplicate. - Return false if a wait occurred and the caller neeeds to re-lock the sync object - and try again. If a duplicate is found release the sync and throw an exception. + Return if the search succeeded by not finding a duplicate. + Retry if a wait occurred. + If a duplicate is found, an exception should be caught by the caller. **/ -bool Table::checkUniqueIndexes(Transaction *transaction, RecordVersion *record, Sync *sync) +void Table::checkUniqueIndexes(Transaction *transaction, RecordVersion *record) { Record *oldRecord = record->priorVersion; + bool retry = true; - FOR_INDEXES(index, this); - if (INDEX_IS_UNIQUE(index->type) && - (!oldRecord || index->changed(record, oldRecord))) + while (retry) + { + retry = false; + FOR_INDEXES(index, this); { - bool noConflict = checkUniqueIndex(index, transaction, record, sync); - - if (!noConflict) - return false; + if (INDEX_IS_UNIQUE(index->type) && + (!oldRecord || index->changed(record, oldRecord))) + { + retry = checkUniqueIndex(index, transaction, record); + if (retry) + break; + } + if (retry) + break; } - END_FOR; - - return true; + END_FOR; + } + + return; } /** @brief Determine if the record we intend to write will have a duplicate conflict with any pending or visible records within a single index. @details For each record number found in a scanIndex, call checkUniqueRecordVersion. - Return same as checkUniqueIndexes. + Return true if a wait occured. + Return false if no duplicate was found. **/ -bool Table::checkUniqueIndex(Index *index, Transaction *transaction, RecordVersion *record, Sync *sync) +bool Table::checkUniqueIndex(Index *index, Transaction *transaction, RecordVersion *record) { Bitmap bitmap; IndexKey indexKey(index); @@ -2380,27 +2378,25 @@ bool Table::checkUniqueIndex(Index *inde for (int32 recordNumber = 0; (recordNumber = bitmap.nextSet(recordNumber)) >= 0; ++recordNumber) { - int rc = checkUniqueRecordVersion(recordNumber, index, transaction, record, sync); + int retry = checkUniqueRecordVersion(recordNumber, index, transaction, record); - if (rc == checkUniqueWaited) - return false; // restart the search with a new lock - - if (rc == checkUniqueIsDone) - return true; // No need to search any more record versions. - // else rc == checkUniqueNext + if (retry) + return true; // restart the search since a wait occurred. } - return true; // Did not find a duplicate + return false; // Did not find a duplicate in this index } /** @brief Determine if the record we intend to write will have a duplicate conflict with any pending or visible recordVersions for a single index and record Number. @details Search through the record version , call checkUniqueRecordVersion. - Return same as checkUniqueIndexes. + Return true if a wait occured. + Return false if no duplicate was found. + Throw an exception if a duplicate WAS found **/ -int Table::checkUniqueRecordVersion(int32 recordNumber, Index *index, Transaction *transaction, RecordVersion *record, Sync *sync) +bool Table::checkUniqueRecordVersion(int32 recordNumber, Index *index, Transaction *transaction, RecordVersion *record) { Record *rec; Record *oldRecord = record->priorVersion; @@ -2408,7 +2404,7 @@ int Table::checkUniqueRecordVersion(int3 State state = CommittedVisible; if (oldRecord && recordNumber == oldRecord->recordNumber) - return checkUniqueNext; // Check next record number. + return false; // Check next record number. // This flag is used to skip all records in the chain between the // first younger committed record and the first older committed record. @@ -2416,7 +2412,7 @@ int Table::checkUniqueRecordVersion(int3 bool foundFirstCommitted = false; if ( !(rec = fetch(recordNumber)) ) - return checkUniqueNext; // Check next record number. + return false; // Check next record number. for (Record *dup = rec; dup; dup = dup->getPriorVersion()) { @@ -2452,7 +2448,7 @@ int Table::checkUniqueRecordVersion(int3 if (activeTransaction) activeTransaction->release(); - return checkUniqueNext; // Check next record number. + return false; // Check next record number. case CommittedInvisible: // This state only happens for consistent read @@ -2491,9 +2487,6 @@ int Table::checkUniqueRecordVersion(int3 { // wait for that transaction, then restart checkUniqueIndexes() - if (sync) - sync->unlock(); - state = transaction->getRelativeState(dup, WAIT_IF_ACTIVE); if (state != Deadlock) @@ -2503,15 +2496,12 @@ int Table::checkUniqueRecordVersion(int3 if (activeTransaction) activeTransaction->release(); - return checkUniqueWaited; + return true; // retry after a wait } } else if (activeTransaction) { - if (sync) - sync->unlock(); - state = transaction->getRelativeState(activeTransaction, activeTransaction->transactionId, WAIT_IF_ACTIVE); @@ -2520,7 +2510,7 @@ int Table::checkUniqueRecordVersion(int3 activeTransaction->release(); rec->release(); - return checkUniqueWaited; + return true; // retry after a wait } } @@ -2576,7 +2566,7 @@ int Table::checkUniqueRecordVersion(int3 if (activeTransaction) activeTransaction->release(); - return checkUniqueNext; // Check next record number. + return false; // Check next record number. } if (state == CommittedInvisible) @@ -2589,7 +2579,7 @@ int Table::checkUniqueRecordVersion(int3 if (activeTransaction) activeTransaction->release(); - return checkUniqueNext; // Check next record number + return false; // Check next record number } } // for each record version... @@ -2599,7 +2589,7 @@ int Table::checkUniqueRecordVersion(int3 if (activeTransaction) activeTransaction->release(); - return checkUniqueNext; + return false; // Check next record number } bool Table::dropForeignKey(int fieldCount, Field **fields, Table *references) @@ -2913,25 +2903,20 @@ uint Table::insert(Transaction *transact if (indexes) { - do - sync.lock(ATOMIC_UPDATE); - while (!checkUniqueIndexes(transaction, record, &sync)); + checkUniqueIndexes(transaction, record); FOR_INDEXES(index, this); index->insert (record, transaction); END_FOR; } - // Do actual insert - + // Do the actual insert + transaction->addRecord(record); bool ret = insert(record, NULL, recordNumber); ASSERT(ret); inserted = true; - - if (indexes) - sync.unlock(); - + record->release(); } catch (...) @@ -3040,13 +3025,9 @@ void Table::update(Transaction * transac // Make insert/update atomic, then check for unique index duplicats - Sync sync(&syncUpdate, "Table::update"); - if (indexes) { - do - sync.lock(ATOMIC_UPDATE); - while (!checkUniqueIndexes(transaction, record, &sync)); + checkUniqueIndexes(transaction, record); FOR_INDEXES(index, this); index->update(oldRecord, record, transaction); @@ -3063,19 +3044,18 @@ void Table::update(Transaction * transac validateAndInsert(transaction, record); transaction->addRecord(record); } - - updated = true; + updated = true; //fireTriggers(transaction, PostUpdate, oldRecord, record); // If this is a re-update in the same transaction and the same savepoint, // carefully remove the prior version. - + record->scavenge(transaction->transactionId, transaction->curSavePointId); - + if (record) record->release(); - + oldRecord->release(); // This reference originated in this function. } catch (...) @@ -3390,12 +3370,19 @@ Record* Table::fetchForUpdate(Transactio return prior; } - Sync sync(&syncObject, "Table::fetchForUpdate"); - - // We need to lock the record - for (;;) { + // Try to avoid getting a lock if there is no way we will be updating this record. + + if (!transaction->needToLock(record)) + { + record->release(); + + return NULL; + } + + // We may need to lock the record + State state = transaction->getRelativeState(record, WAIT_IF_ACTIVE); switch (state) @@ -3405,7 +3392,8 @@ Record* Table::fetchForUpdate(Transactio ASSERT(IS_CONSISTENT_READ(transaction->isolationLevel)); record->release(); - Log::debug("Table::fetchForUpdate: update conflict in table %s.%s", schemaName, name); + Log::debug("Table::fetchForUpdate: Update Conflict: TransId=%d, RecordNumber=%d, Table %s.%s", + transaction->transactionId, record->recordNumber, schemaName, name); throw SQLError(UPDATE_CONFLICT, "update conflict in table %s.%s", schemaName, name); case CommittedVisible: @@ -3413,15 +3401,18 @@ Record* Table::fetchForUpdate(Transactio if (record->state == recDeleted) { record->release(); - + return NULL; } // Lock the record + if (dbb->debug & DEBUG_RECORD_LOCKS) + Log::debug("Table::fetchForUpdate: TransactionId=%d, isolationLevel=%d, recordNumber=%d\n", + transaction->transactionId, transaction->isolationLevel, recordNumber); + RecordVersion *recordVersion = allocRecordVersion(NULL, transaction, record); recordVersion->state = recLock; - //sync.lock(Exclusive); if (insert(recordVersion, record, recordNumber)) { @@ -3436,7 +3427,6 @@ Record* Table::fetchForUpdate(Transactio return record; } - //sync.unlock(); recordVersion->active = false; recordVersion->release(); } diff -Nrup a/storage/falcon/Table.h b/storage/falcon/Table.h --- a/storage/falcon/Table.h 2008-02-15 16:29:49 -06:00 +++ b/storage/falcon/Table.h 2008-02-28 12:18:40 -06:00 @@ -40,10 +40,6 @@ static const int PostDelete = 32; static const int PreCommit = 64; static const int PostCommit = 128; -static const int checkUniqueWaited = 0; -static const int checkUniqueIsDone = 1; -static const int checkUniqueNext = 2; - #define FORMAT_HASH_SIZE 20 #define FOR_FIELDS(field,table) {for (Field *field=table->fields; field; field = field->next){ #define FOR_INDEXES(index,table) {for (Index *index=table->indexes; index; index = index->next){ @@ -103,9 +99,9 @@ public: bool foreignKeyMember (ForeignKey *key); void makeNotSearchable (Field *field, Transaction *transaction); bool dropForeignKey (int fieldCount, Field **fields, Table *references); - bool checkUniqueIndexes (Transaction *transaction, RecordVersion *record, Sync *sync); - bool checkUniqueIndex(Index *index, Transaction *transaction, RecordVersion *record, Sync *sync); - int checkUniqueRecordVersion(int32 recordNumber, Index *index, Transaction *transaction, RecordVersion *record, Sync *sync); + void checkUniqueIndexes (Transaction *transaction, RecordVersion *record); + bool checkUniqueIndex(Index *index, Transaction *transaction, RecordVersion *record); + bool checkUniqueRecordVersion(int32 recordNumber, Index *index, Transaction *transaction, RecordVersion *record); bool isDuplicate (Index *index, Record *record1, Record *record2); void checkDrop(); Field* findField (const WCString *fieldName); diff -Nrup a/storage/falcon/Transaction.cpp b/storage/falcon/Transaction.cpp --- a/storage/falcon/Transaction.cpp 2008-02-15 16:43:54 -06:00 +++ b/storage/falcon/Transaction.cpp 2008-02-28 12:18:47 -06:00 @@ -363,7 +363,7 @@ void Transaction::commitNoUpdates(void) thread = Thread::getThread("Transaction::commitNoUpdates"); } - ASSERT(dependencies == 0); + ASSERT(dependencies == 0); sync.unlock(); delete [] xid; xid = NULL; @@ -720,6 +720,34 @@ bool Transaction::visible(Transaction * return true; } +/*** +@brief Determine if there is a need to lock this record for update. +***/ + +bool Transaction::needToLock(Record* record) +{ + // Find the first visible record version + + for (Record* candidate = record; + candidate != NULL; + candidate = candidate->getPriorVersion()) + { + Transaction *transaction = candidate->getTransaction(); + TransId transId = candidate->getTransactionId(); + + if (visible(transaction, transId, FOR_WRITING)) + if (candidate->state == recDeleted) + if (!transaction || transaction->state == Committed) + return FALSE; // Committed and deleted + else + return TRUE; // Just in case this rolls back. + else + return TRUE; + } + + return FALSE; +} + void Transaction::releaseDependencies() { if (!numberStates) @@ -1101,7 +1129,7 @@ void Transaction::rollbackSavepoint(int if (systemTransaction) sync.lock(Exclusive); - // Be sure the target savepoint is valid before rollong them back. + // Be sure the target savepoint is valid before rolling them back. for (savePoint = savePoints; savePoint; savePoint = savePoint->next) if (savePoint->id <= savePointId) diff -Nrup a/storage/falcon/Transaction.h b/storage/falcon/Transaction.h --- a/storage/falcon/Transaction.h 2008-02-15 16:32:11 -06:00 +++ b/storage/falcon/Transaction.h 2008-02-28 12:18:52 -06:00 @@ -94,6 +94,7 @@ public: void commitRecords(); void releaseDependencies(); bool visible (Transaction *transaction, TransId transId, int forWhat); + bool needToLock(Record* record); void addRecord (RecordVersion *record); void prepare(int xidLength, const UCHAR *xid); void rollback();