Truncate range tombstones from sstables #3
Conversation
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @petermattis and @bdarnell)
db/range_del_aggregator.cc, line 217 at r1 (raw file):
// TODO(peter): What is the lifetime smallest and largest? They // are being taken from FileMetadata::{smallest,largest}. Will // they live at least as long as the RangeDelAggregator?
We should resolve this TODO before merging.
petermattis
force-pushed
the
pmattis/truncate-range-tombstones-crl
branch
from
e8bf454 to
6c31640
Compare
There was a problem hiding this comment.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @benesch, @petermattis, and @bdarnell)
db/range_del_aggregator.cc, line 217 at r1 (raw file):
Previously, benesch (Nikhil Benesch) wrote…
We should resolve this TODO before merging.
I tracked down the lifetime of FileMetaData::{smallest,largest}. They are tied to VersionStorageInfo which itself is tied to Version. An iterator pins the Version it is associated with in memory via a reference to a SuperVersion. I believe there is nothing to do here.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @bdarnell)
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewed 9 of 10 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @petermattis)
db/range_del_aggregator.h, line 107 at r2 (raw file):
// Adds tombstones to the tombstone aggregation structure maintained // by this object. Tombstones are truncated to smallest and
This could probably use a comment about inclusive vs exclusive end keys.
petermattis
force-pushed
the
pmattis/truncate-range-tombstones-crl
branch
from
6c31640 to
3e2c751
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @bdarnell and @benesch)
db/range_del_aggregator.h, line 107 at r2 (raw file):
Previously, bdarnell (Ben Darnell) wrote…
This could probably use a comment about inclusive vs exclusive end keys.
Done.
When adding range tombstones to a RangeDelAggregator, truncate the tombstones to the sstable boundaries. This is done as an alternative to truncating the range tombstones within the sstables themselves as it properly handles existing data where range tombstones are not truncated in sstables. Truncating range tombstones to sstable boundaries avoids having two process all of the sstables with overlapping tombstones as a unit. See facebook#4050
petermattis
force-pushed
the
pmattis/truncate-range-tombstones-crl
branch
from
3e2c751 to
c4636e3
Compare
…_family_test (facebook#4474) Summary: this should fix the current failing TSAN jobs: The callstack for TSAN: > WARNING: ThreadSanitizer: data race (pid=87440) Read of size 8 at 0x7d580000fce0 by thread T22 (mutexes: write M548703): #0 rocksdb::InternalStats::DumpCFStatsNoFileHistogram(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) db/internal_stats.cc:1204 (column_family_test+0x00000080eca7) #1 rocksdb::InternalStats::DumpCFStats(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) db/internal_stats.cc:1169 (column_family_test+0x0000008106d0) #2 rocksdb::InternalStats::HandleCFStats(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, rocksdb::Slice) db/internal_stats.cc:578 (column_family_test+0x000000810720) #3 rocksdb::InternalStats::GetStringProperty(rocksdb::DBPropertyInfo const&, rocksdb::Slice const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) db/internal_stats.cc:488 (column_family_test+0x00000080670c) #4 rocksdb::DBImpl::DumpStats() db/db_impl.cc:625 (column_family_test+0x00000070ce9a) > Previous write of size 8 at 0x7d580000fce0 by main thread: #0 rocksdb::InternalStats::AddCFStats(rocksdb::InternalStats::InternalCFStatsType, unsigned long) db/internal_stats.h:324 (column_family_test+0x000000693bbf) #1 rocksdb::ColumnFamilyData::RecalculateWriteStallConditions(rocksdb::MutableCFOptions const&) db/column_family.cc:818 (column_family_test+0x000000693bbf) #2 rocksdb::ColumnFamilyTest_WriteStallSingleColumnFamily_Test::TestBody() db/column_family_test.cc:2563 (column_family_test+0x0000005e5a49) Pull Request resolved: facebook#4474 Differential Revision: D10262099 Pulled By: miasantreble fbshipit-source-id: 1247973a3ca32e399b4575d3401dd5439c39efc5
[This is a backport to crl-release-5.13 of an upstream PR. I'll do my best to keep the implementations in sync as comments come in on both PRs.]
This PR provides little benefit by itself, but it allows us to relax a compaction heuristic that expands a compaction to include all of the sstables that are overlapped by a range tombstone which can result in excessively large compactions. That change will be performed in a follow-on PR.
This change is