Skip to content

Commit

Permalink
Report corrupted keys during compaction (facebook#7124)
Browse files Browse the repository at this point in the history
Summary:
Currently, RocksDB lets compaction to go through even in case of
corrupted keys, the number of which is reported in CompactionJobStats.
However, RocksDB does not check this value. We should let compaction run
in a stricter mode.

Temporarily disable two tests that allow corrupted keys in compaction.
With this PR, the two tests will assert(false) and terminate. Still need
to investigate what is the recommended google-test way of doing it.
Death test (EXPECT_DEATH) in gtest has warnings now.

Pull Request resolved: facebook#7124

Test Plan: make check

Reviewed By: ajkr

Differential Revision: D22530722

Pulled By: riversand963

fbshipit-source-id: 6a5a6a992028c6d4f92cb74693c92db462ae4ad6
  • Loading branch information
riversand963 authored and facebook-github-bot committed Jul 15, 2020
1 parent 687fbd0 commit 27735de
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 6 deletions.
1 change: 1 addition & 0 deletions HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* Fix a bug of wrong iterator result if another thread finishes an update and a DB flush between two statement.
* Disable file deletion after MANIFEST write/sync failure until db re-open or Resume() so that subsequent re-open will not see MANIFEST referencing deleted SSTs.
* Fix a bug when index_type == kTwoLevelIndexSearch in PartitionedIndexBuilder to update FlushPolicy to point to internal key partitioner when it changes from user-key mode to internal-key mode in index partition.
* Make compaction report InternalKey corruption while iterating over the input.

### Public API Change
* `DB::GetDbSessionId(std::string& session_id)` is added. `session_id` stores a unique identifier that gets reset every time the DB is opened. This DB session ID should be unique among all open DB instances on all hosts, and should be unique among re-openings of the same or other DBs. This identifier is recorded in the LOG file on the line starting with "DB Session ID:".
Expand Down
2 changes: 1 addition & 1 deletion db/compaction/compaction_iterator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ void CompactionIterator::NextFromInput() {
iter_stats_.num_input_records++;

if (!ParseInternalKey(key_, &ikey_)) {
iter_stats_.num_input_corrupt_records++;
// If `expect_valid_internal_key_` is false, return the corrupted key
// and let the caller decide what to do with it.
// TODO(noetzli): We should have a more elegant solution for this.
Expand All @@ -275,7 +276,6 @@ void CompactionIterator::NextFromInput() {
has_current_user_key_ = false;
current_user_key_sequence_ = kMaxSequenceNumber;
current_user_key_snapshot_ = 0;
iter_stats_.num_input_corrupt_records++;
valid_ = true;
break;
}
Expand Down
7 changes: 4 additions & 3 deletions db/compaction/compaction_job.cc
Original file line number Diff line number Diff line change
Expand Up @@ -898,9 +898,10 @@ void CompactionJob::ProcessKeyValueCompaction(SubcompactionState* sub_compact) {
sub_compact->c_iter.reset(new CompactionIterator(
input.get(), cfd->user_comparator(), &merge, versions_->LastSequence(),
&existing_snapshots_, earliest_write_conflict_snapshot_,
snapshot_checker_, env_, ShouldReportDetailedTime(env_, stats_), false,
&range_del_agg, sub_compact->compaction, compaction_filter,
shutting_down_, preserve_deletes_seqnum_, manual_compaction_paused_,
snapshot_checker_, env_, ShouldReportDetailedTime(env_, stats_),
/*expect_valid_internal_key=*/true, &range_del_agg,
sub_compact->compaction, compaction_filter, shutting_down_,
preserve_deletes_seqnum_, manual_compaction_paused_,
db_options_.info_log));
auto c_iter = sub_compact->c_iter.get();
c_iter->SeekToFirst();
Expand Down
4 changes: 2 additions & 2 deletions db/compaction/compaction_job_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ TEST_F(CompactionJobTest, Simple) {
RunCompaction({ files }, expected_results);
}

TEST_F(CompactionJobTest, SimpleCorrupted) {
TEST_F(CompactionJobTest, DISABLED_SimpleCorrupted) {
NewDB();

auto expected_results = CreateTwoFiles(true);
Expand Down Expand Up @@ -989,7 +989,7 @@ TEST_F(CompactionJobTest, MultiSingleDelete) {
// single deletion and the (single) deletion gets removed while the corrupt key
// gets written out. TODO(noetzli): We probably want a better way to treat
// corrupt keys.
TEST_F(CompactionJobTest, CorruptionAfterDeletion) {
TEST_F(CompactionJobTest, DISABLED_CorruptionAfterDeletion) {
NewDB();

auto file1 =
Expand Down

0 comments on commit 27735de

Please sign in to comment.