#At file:///home/cpowers/work/dev/dev-07/mysql/
2845 Christopher Powers 2008-10-01
Bug#39711 "Running falcon_bug_34351_C shows increasing memory usage"
Bug#39795 "Falcon: Online add index does not support index with non-null columns"
Bug#39796 "Falcon: Reference count decrement not atomic"
modified:
storage/falcon/DeferredIndex.cpp
storage/falcon/DeferredIndex.h
storage/falcon/DeferredIndexWalker.cpp
storage/falcon/Index.cpp
storage/falcon/Transaction.cpp
storage/falcon/Transaction.h
storage/falcon/WalkDeferred.cpp
storage/falcon/ha_falcon.cpp
per-file messages:
storage/falcon/DeferredIndex.cpp
Renamed ::releaseRef() to ::release() to be consistent with other classes
Corrected refcount atomicity in ::release()
storage/falcon/DeferredIndex.h
Renamed ::releaseRef() to ::release() to be consistent with other classes
storage/falcon/DeferredIndexWalker.cpp
Renamed DeferredIndex::releaseRef() to ::release() to be consistent with other classes
storage/falcon/Index.cpp
Added deferredIndex->release() to Index::detachDeferredIndex()
Note that this is the correct location for release(), but it is temporarily disabled until the DeferredIndex refcount analysis is complete.
storage/falcon/Transaction.cpp
Disabled DeferredIndex::addRef() in Transaction::add(DefererredIndex*) to avoid DeferredIndex memory leak. Note that this is an intermediate solution.
Renamed DeferredIndex::releaseRef() to ::release() to be consistent with other classes
Corrected refcount in ::release() to ensure atomicity
storage/falcon/Transaction.h
Declared Transaction::release() as void
storage/falcon/WalkDeferred.cpp
Renamed DeferredIndex::releaseRef() to ::release() to be consistent with other classes
Added comment to WalkDefered constructor indicating where DeferredIndex::addRef() should be located once refcount analysis is complete.
storage/falcon/ha_falcon.cpp
Remove nullable column and primary key restrictions for online index creation in StorageInterface::check_if_supported_alter()
=== modified file 'storage/falcon/DeferredIndex.cpp'
--- a/storage/falcon/DeferredIndex.cpp 2008-09-10 19:51:03 +0000
+++ b/storage/falcon/DeferredIndex.cpp 2008-10-02 00:09:30 +0000
@@ -840,7 +840,7 @@ void DeferredIndex::detachTransaction(vo
else
sync.unlock();
- //releaseRef();
+ //release();
}
void DeferredIndex::chill(Dbb *dbb)
@@ -882,13 +882,9 @@ void DeferredIndex::addRef()
INTERLOCKED_INCREMENT (useCount);
}
-void DeferredIndex::releaseRef()
+void DeferredIndex::release()
{
- ASSERT(useCount > 0);
-
- INTERLOCKED_DECREMENT(useCount);
-
- if (useCount == 0)
+ if (INTERLOCKED_DECREMENT(useCount) == 0)
delete this;
}
=== modified file 'storage/falcon/DeferredIndex.h'
--- a/storage/falcon/DeferredIndex.h 2008-07-25 18:07:24 +0000
+++ b/storage/falcon/DeferredIndex.h 2008-10-02 00:09:30 +0000
@@ -125,7 +125,7 @@ public:
uint64 virtualOffsetAtEnd;
SerialLogWindow *window;
void addRef();
- void releaseRef();
+ void release();
};
#endif
=== modified file 'storage/falcon/DeferredIndexWalker.cpp'
--- a/storage/falcon/DeferredIndexWalker.cpp 2008-09-10 19:51:03 +0000
+++ b/storage/falcon/DeferredIndexWalker.cpp 2008-10-02 00:09:30 +0000
@@ -96,7 +96,7 @@ void DeferredIndexWalker::initialize(Def
DeferredIndexWalker::~DeferredIndexWalker(void)
{
if (deferredIndex)
- deferredIndex->releaseRef();
+ deferredIndex->release();
}
DINode* DeferredIndexWalker::next(void)
=== modified file 'storage/falcon/Index.cpp'
--- a/storage/falcon/Index.cpp 2008-08-19 03:33:01 +0000
+++ b/storage/falcon/Index.cpp 2008-10-02 00:09:30 +0000
@@ -514,7 +514,7 @@ IndexWalker* Index::positionIndex(IndexK
if (transaction->visible(deferredIndex->transaction, deferredIndex->transactionId, FOR_WRITING))
{
deferredIndex->addRef();
-
+
if (!indexWalker)
{
indexWalker = new IndexWalker(this, transaction, searchFlags);
@@ -843,6 +843,7 @@ void Index::detachDeferredIndex(Deferred
Sync sync(&deferredIndexes.syncObject, "Index::detachDeferredIndex(1)");
sync.lock(Exclusive);
deferredIndexes.remove(deferredIndex);
+// deferredIndex->release(); // temp disabled for Bug#39711, still causes crash
sync.unlock();
if ( (database->configuration->useDeferredIndexHash)
@@ -861,7 +862,6 @@ void Index::detachDeferredIndex(Deferred
removeFromDIHash(uniqueNode);
}
}
-
}
int Index::getRootPage(void)
=== modified file 'storage/falcon/Transaction.cpp'
--- a/storage/falcon/Transaction.cpp 2008-09-10 19:51:03 +0000
+++ b/storage/falcon/Transaction.cpp 2008-10-02 00:09:30 +0000
@@ -1076,14 +1076,10 @@ void Transaction::addRef()
INTERLOCKED_INCREMENT(useCount);
}
-int Transaction::release()
+void Transaction::release()
{
- int count = INTERLOCKED_DECREMENT(useCount);
-
- if (count == 0)
+ if (INTERLOCKED_DECREMENT(useCount) == 0)
delete this;
-
- return count;
}
int Transaction::createSavepoint()
@@ -1317,7 +1313,7 @@ void Transaction::add(DeferredIndex* def
Sync sync(&syncDeferredIndexes, "Transaction::add");
sync.lock(Exclusive);
- deferredIndex->addRef();
+// deferredIndex->addRef(); // temp disable for Bug#39711 memory leak (see Index::detachDeferredIndex)
deferredIndex->nextInTransaction = deferredIndexes;
deferredIndexes = deferredIndex;
deferredIndexCount++;
@@ -1504,7 +1500,7 @@ void Transaction::releaseDeferredIndexes
ASSERT(deferredIndex->transaction == this);
deferredIndexes = deferredIndex->nextInTransaction;
deferredIndex->detachTransaction();
- deferredIndex->releaseRef();
+ deferredIndex->release();
deferredIndexCount--;
}
}
@@ -1520,7 +1516,7 @@ void Transaction::releaseDeferredIndexes
{
*ptr = deferredIndex->nextInTransaction;
deferredIndex->detachTransaction();
- deferredIndex->releaseRef();
+ deferredIndex->release();
--deferredIndexCount;
}
else
=== modified file 'storage/falcon/Transaction.h'
--- a/storage/falcon/Transaction.h 2008-09-10 04:02:07 +0000
+++ b/storage/falcon/Transaction.h 2008-10-02 00:09:30 +0000
@@ -99,7 +99,7 @@ public:
void prepare(int xidLength, const UCHAR *xid);
void rollback();
void commit();
- int release();
+ void release();
void addRef();
void waitForTransaction();
bool waitForTransaction (TransId transId);
=== modified file 'storage/falcon/WalkDeferred.cpp'
--- a/storage/falcon/WalkDeferred.cpp 2008-07-25 18:07:24 +0000
+++ b/storage/falcon/WalkDeferred.cpp 2008-10-02 00:09:30 +0000
@@ -19,6 +19,7 @@
WalkDeferred::WalkDeferred(DeferredIndex *deferredIdx, Transaction *transaction, int flags, IndexKey *lower, IndexKey *upper)
: IndexWalker(deferredIdx->index, transaction, flags)
{
+ //deferredIdx->addRef(); // assume addref by caller
deferredIndex = deferredIdx;
walker.initialize(deferredIndex, lower, flags);
node = NULL;
@@ -26,7 +27,7 @@ WalkDeferred::WalkDeferred(DeferredIndex
WalkDeferred::~WalkDeferred(void)
{
- deferredIndex->releaseRef();
+ deferredIndex->release();
}
Record* WalkDeferred::getNext(bool lockForUpdate)
=== modified file 'storage/falcon/ha_falcon.cpp'
--- a/storage/falcon/ha_falcon.cpp 2008-09-16 17:58:49 +0000
+++ b/storage/falcon/ha_falcon.cpp 2008-10-02 00:09:30 +0000
@@ -2186,34 +2186,6 @@ int StorageInterface::check_if_supported
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++)
- {
- if (n != altered_table->s->primary_key)
- {
- KEY *key = altered_table->key_info + n;
- KEY *tableEnd = table->key_info + table->s->keys;
- KEY *tableKey;
-
- // Determine if this is a new index
-
- for (tableKey = table->key_info; tableKey < tableEnd; tableKey++)
- if (!strcmp(tableKey->name, key->name))
- break;
-
- // Verify that each part is nullable
-
- if (tableKey >= tableEnd)
- for (uint p = 0; p < key->key_parts; p++)
- {
- KEY_PART_INFO *keyPart = key->key_part + p;
- if (keyPart && !keyPart->field->real_maybe_null())
- {
- DBUG_PRINT("info",("Online add index columns must be nullable"));
- DBUG_RETURN(HA_ALTER_NOT_SUPPORTED);
- }
- }
- }
- }
}
if (alter_flags->is_set(HA_DROP_INDEX) || alter_flags->is_set(HA_DROP_UNIQUE_INDEX))
@@ -2307,20 +2279,17 @@ int StorageInterface::addIndex(THD* thd,
for (unsigned int n = 0; n < alteredTable->s->keys; n++)
{
- if (n != alteredTable->s->primary_key)
- {
- KEY *key = alteredTable->key_info + n;
- KEY *tableEnd = table->key_info + table->s->keys;
- KEY *tableKey;
+ KEY *key = alteredTable->key_info + n;
+ KEY *tableEnd = table->key_info + table->s->keys;
+ KEY *tableKey;
- for (tableKey = table->key_info; tableKey < tableEnd; tableKey++)
- if (!strcmp(tableKey->name, key->name))
- break;
+ for (tableKey = table->key_info; tableKey < tableEnd; tableKey++)
+ if (!strcmp(tableKey->name, key->name))
+ break;
- if (tableKey >= tableEnd)
- if ((ret = createIndex(schemaName, tableName, alteredTable, n)))
- break;
- }
+ if (tableKey >= tableEnd)
+ if ((ret = createIndex(schemaName, tableName, alteredTable, n)))
+ break;
}
// The server indexes may have been reordered, so remap to the Falcon indexes
@@ -2349,20 +2318,17 @@ int StorageInterface::dropIndex(THD* thd
for (unsigned int n = 0; n < table->s->keys; n++)
{
- if (n != table->s->primary_key)
- {
- KEY *key = table->key_info + n;
- KEY *alterEnd = alteredTable->key_info + alteredTable->s->keys;
- KEY *alterKey;
+ KEY *key = table->key_info + n;
+ KEY *alterEnd = alteredTable->key_info + alteredTable->s->keys;
+ KEY *alterKey;
- for (alterKey = alteredTable->key_info; alterKey < alterEnd; alterKey++)
- if (!strcmp(alterKey->name, key->name))
- break;
-
- if (alterKey >= alterEnd)
- if ((ret = dropIndex(schemaName, tableName, table, n)))
- break;
- }
+ for (alterKey = alteredTable->key_info; alterKey < alterEnd; alterKey++)
+ if (!strcmp(alterKey->name, key->name))
+ break;
+
+ if (alterKey >= alterEnd)
+ if ((ret = dropIndex(schemaName, tableName, table, n)))
+ break;
}
// The server indexes have been reordered, so remap to the Falcon indexes
