From: Date: November 20 2008 4:51pm Subject: Re: bzr commit into mysql-6.0-falcon-team branch (klewis:2912) List-Archive: http://lists.mysql.com/commits/59408 Message-Id: <49258770.9080706@sun.com> MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset=ISO-8859-1 Content-Transfer-Encoding: 7BIT Olave wrote; ------------------------ Hi Kevin, I have read and tried to understand the patch. To me it seems correct and I also think it makes to code simpler and more readable. My only comment is that you should update the following comment in: // If the record is locked or being unlocked keep looking for a dup. in Table::checkUniqueRecordVersion. If you plan to push the patch, let me know and I can respond to that I have reviewed it on the commit list. Olav ---------------------- Thanks, Olav. OK, I will push it. I think I will also open a bug to track this, even though we do not yet know what effects this would have, and what other bugs it might fix. Kevin Kevin Lewis wrote: > #At file:///C:/Work/bzr/Merge/mysql-6.0-falcon-team/ > > 2912 Kevin Lewis 2008-11-19 > Do not use Record::state = recUnlock. It is not > recognized by many parts of the code and can > cause the wrong behavior. > > Also, A lock record that is unlocked should stay > on the transaction. It is still replaced on the > recordLeaf, but other threads with pointers to it > from a concurrent Table::fetch need to be able to > see the transaction. It will be taken off the > transaction later with all the other records on > the transaction. > modified: > storage/falcon/Table.cpp > storage/falcon/Table.h > storage/falcon/Transaction.cpp > > per-file messages: > storage/falcon/Table.cpp > Don't search for recUnlock. It is no longer used. > The 'remove' parameter is no longer needed in > Table::unlockRecord() since lock records are no > longer removed from the transaction. > Use superceded=true to keep from calling > insert again if Table::unlockRecord() is called > a second time. This can now happen during a > Transaction::rollback() after a rollbackSavepoint > since unlocked records now remain on the transaction. > storage/falcon/Table.h > The 'remove' parameter is no longer needed in > Table::unlockRecord() since lock records are no > longer removed from the transaction. > storage/falcon/Transaction.cpp > The 'remove' parameter is no longer needed in > Table::unlockRecord() since lock records are no > longer removed from the transaction. > > Transaction::releaseRecordLocks() needs to loop > differently since the records are no longer > removed from the transaction. They are removed > later with all the other records. > === modified file 'storage/falcon/Table.cpp' > --- a/storage/falcon/Table.cpp 2008-10-24 05:35:38 +0000 > +++ b/storage/falcon/Table.cpp 2008-11-20 05:32:18 +0000 > @@ -2555,7 +2555,7 @@ bool Table::checkUniqueRecordVersion(int > { > // If the record is locked or being unlocked keep looking for a dup. > > - if ((dup->state == recLock) || (dup->state == recUnlocked)) > + if (dup->state == recLock) > continue; // Next record version. > > // The record has been deleted. > @@ -3436,26 +3436,23 @@ void Table::unlockRecord(int recordNumbe > if (record) > { > if (record->state == recLock) > - unlockRecord((RecordVersion*) record, true); > + unlockRecord((RecordVersion*) record); > > record->release(); > } > } > > -void Table::unlockRecord(RecordVersion* record, bool remove) > +void Table::unlockRecord(RecordVersion* record) > { > //int uc = record->useCount; > ASSERT(record->getPriorVersion()); > - > - if (record->state == recLock) > - { > - record->state = recUnlocked; > > + // A lock record that has superceded=true is already unlocked > + > + if ((record->state == recLock) && !record->isSuperceded()) > + { > if (insert(record->getPriorVersion(), record, record->recordNumber)) > - { > - if (remove && record->transaction) > - record->transaction->removeRecord(record); > - } > + record->setSuperceded(true); > else > Log::debug("Table::unlockRecord: record lock not in record tree\n"); > } > > === modified file 'storage/falcon/Table.h' > --- a/storage/falcon/Table.h 2008-10-24 05:35:38 +0000 > +++ b/storage/falcon/Table.h 2008-11-20 05:32:18 +0000 > @@ -208,7 +208,7 @@ public: > Record* fetchForUpdate(Transaction* transaction, Record* record, bool usingIndex); > // RecordVersion* lockRecord(Record* record, Transaction* transaction); > void unlockRecord(int recordNumber); > - void unlockRecord(RecordVersion* record, bool remove); > + void unlockRecord(RecordVersion* record); > > void insert (Transaction *transaction, int count, Field **fields, Value **values); > uint insert (Transaction *transaction, Stream *stream); > > === modified file 'storage/falcon/Transaction.cpp' > --- a/storage/falcon/Transaction.cpp 2008-11-07 01:09:04 +0000 > +++ b/storage/falcon/Transaction.cpp 2008-11-20 05:32:18 +0000 > @@ -422,7 +422,7 @@ void Transaction::rollback() > record->nextInTrans = NULL; > > if (record->state == recLock) > - record->format->table->unlockRecord(record, false); > + record->format->table->unlockRecord(record); > else > record->rollback(this); > > @@ -1353,19 +1353,19 @@ bool Transaction::isXidEqual(int testLen > > void Transaction::releaseRecordLocks(void) > { > - RecordVersion **ptr; > - RecordVersion *record; > - > Sync syncRec(&syncRecords,"Transaction::releaseRecordLocks"); > syncRec.lock(Exclusive); > - for (ptr = &firstRecord; (record = *ptr);) > + > + for (RecordVersion *record = firstRecord; record; record = record->nextInTrans) > if (record->state == recLock) > { > - record->format->table->unlockRecord(record, false); > - removeRecord(record); > + record->format->table->unlockRecord(record); > + > + // Don't do removeRecord(record); now. Other threads might be > + // pointing to it and need the transaction pointer to determine > + // its relative state > } > - else > - ptr = &record->nextInTrans; > + > syncRec.unlock(); > } > > >