andreacavalli/rocksdb
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

fix asan/valgrind for TableCache cleanup

Browse Source 
Summary:
Breaking commit: d12691b86f

In the above commit, I moved the `TableCache` cleanup logic from `Version` destructor into `PurgeObsoleteFiles`. I missed cleaning up `TableCache` entries for the current `Version` during DB destruction.

This PR adds that logic to `VersionSet` destructor. One unfortunate side effect is now we're potentially deleting `TableReader`s after `column_family_set_.reset()`, which means we can't call `BlockBasedTableReader::Close` a second time as the block cache might already be destroyed.
Closes https://github.com/facebook/rocksdb/pull/2662

Differential Revision: D5515108

Pulled By: ajkr

fbshipit-source-id: 2cb820e19aa813e0d258d17f76b2d7b6b7ee0b18
This commit is contained in:
Andrew Kryczka 2017-07-27 20:16:25 -07:00 committed by Facebook Github Bot
parent 3a3fb00b7a
commit 710411aea6
3 changed files with 11 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
db/version_set.cc
View File

@ -2327,10 +2327,14 @@ void CloseTables(void* ptr, size_t) {
VersionSet::~VersionSet() { VersionSet::~VersionSet() {
// we need to delete column_family_set_ because its destructor depends on // we need to delete column_family_set_ because its destructor depends on
// VersionSet // VersionSet
column_family_set_->get_table_cache()->ApplyToAllCacheEntries(&CloseTables, Cache* table_cache = column_family_set_->get_table_cache();
false); table_cache->ApplyToAllCacheEntries(&CloseTables, false /* thread_safe */);
column_family_set_.reset(); column_family_set_.reset();
for (auto file : obsolete_files_) { for (auto file : obsolete_files_) {
if (file->table_reader_handle) {
table_cache->Release(file->table_reader_handle);
TableCache::Evict(table_cache, file->fd.GetNumber());
}
delete file; delete file;
} }
obsolete_files_.clear(); obsolete_files_.clear();

4
table/block_based_table_reader.cc
View File

@ -2083,6 +2083,9 @@ Status BlockBasedTable::DumpTable(WritableFile* out_file) {
} }
void BlockBasedTable::Close() { void BlockBasedTable::Close() {
if (rep_->closed) {
return;
}
rep_->filter_entry.Release(rep_->table_options.block_cache.get()); rep_->filter_entry.Release(rep_->table_options.block_cache.get());
rep_->index_entry.Release(rep_->table_options.block_cache.get()); rep_->index_entry.Release(rep_->table_options.block_cache.get());
rep_->range_del_entry.Release(rep_->table_options.block_cache.get()); rep_->range_del_entry.Release(rep_->table_options.block_cache.get());
@ -2099,6 +2102,7 @@ void BlockBasedTable::Close() {
rep_->dummy_index_reader_offset, cache_key); rep_->dummy_index_reader_offset, cache_key);
rep_->table_options.block_cache.get()->Erase(key); rep_->table_options.block_cache.get()->Erase(key);
} }
rep_->closed = true;
} }
Status BlockBasedTable::DumpIndexBlock(WritableFile* out_file) { Status BlockBasedTable::DumpIndexBlock(WritableFile* out_file) {

1
table/block_based_table_reader.h
View File

@ -467,6 +467,7 @@ struct BlockBasedTable::Rep {
// A value of kDisableGlobalSequenceNumber means that this feature is disabled // A value of kDisableGlobalSequenceNumber means that this feature is disabled
// and every key have it's own seqno. // and every key have it's own seqno.
SequenceNumber global_seqno; SequenceNumber global_seqno;
bool closed = false;
}; };
} // namespace rocksdb } // namespace rocksdb