From e5311a8ea4ed5ca982f0fe9bbfec15d42cff3593 Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Fri, 29 Jan 2021 12:39:44 -0800 Subject: [PATCH] Fix a SingleDelete related optimization for blob indexes (#7904) Summary: There is a small `SingleDelete` related optimization in the `CompactionIterator` code: when a `SingleDelete`-`Put` pair is preserved solely for the purposes of transaction conflict checking, the value itself gets cleared. (This is referred to as "optimization 3" in the `CompactionIterator` code.) Though the rest of the code got updated to support `SingleDelete`'ing blob indexes, this chunk was apparently missed, resulting in an assertion failure (or `ROCKS_LOG_FATAL` in release builds) when triggered. Note: in addition to clearing the value, we also need to update the type of the KV to regular value when dealing with blob indexes here. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7904 Test Plan: `make check` Reviewed By: ajkr Differential Revision: D26118009 Pulled By: ltamasi fbshipit-source-id: 6bf78043d20265e2b15c2e1ab8865025040c42ae --- db/compaction/compaction_iterator.cc | 9 +++++++-- db/compaction/compaction_iterator_test.cc | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/db/compaction/compaction_iterator.cc b/db/compaction/compaction_iterator.cc index cbe9291ce..02ce7818e 100644 --- a/db/compaction/compaction_iterator.cc +++ b/db/compaction/compaction_iterator.cc @@ -423,8 +423,8 @@ void CompactionIterator::NextFromInput() { // In the previous iteration we encountered a single delete that we could // not compact out. We will keep this Put, but can drop it's data. // (See Optimization 3, below.) - assert(ikey_.type == kTypeValue); - if (ikey_.type != kTypeValue) { + assert(ikey_.type == kTypeValue || ikey_.type == kTypeBlobIndex); + if (ikey_.type != kTypeValue && ikey_.type != kTypeBlobIndex) { ROCKS_LOG_FATAL(info_log_, "Unexpected key type %d for compaction output", ikey_.type); @@ -437,6 +437,11 @@ void CompactionIterator::NextFromInput() { current_user_key_snapshot_, last_snapshot); } + if (ikey_.type == kTypeBlobIndex) { + ikey_.type = kTypeValue; + current_key_.UpdateInternalKey(ikey_.sequence, ikey_.type); + } + value_.clear(); valid_ = true; clear_and_output_next_key_ = false; diff --git a/db/compaction/compaction_iterator_test.cc b/db/compaction/compaction_iterator_test.cc index acd3b6600..12a427075 100644 --- a/db/compaction/compaction_iterator_test.cc +++ b/db/compaction/compaction_iterator_test.cc @@ -970,6 +970,21 @@ TEST_F(CompactionIteratorWithSnapshotCheckerTest, 2 /*earliest_write_conflict_snapshot*/); } +// Same as above but with a blob index. In addition to the value getting +// trimmed, the type of the KV is changed to kTypeValue. +TEST_F(CompactionIteratorWithSnapshotCheckerTest, + KeepSingleDeletionForWriteConflictChecking_BlobIndex) { + AddSnapshot(2, 0); + RunTest({test::KeyStr("a", 2, kTypeSingleDeletion), + test::KeyStr("a", 1, kTypeBlobIndex)}, + {"", "fake_blob_index"}, + {test::KeyStr("a", 2, kTypeSingleDeletion), + test::KeyStr("a", 1, kTypeValue)}, + {"", ""}, 2 /*last_committed_seq*/, nullptr /*merge_operator*/, + nullptr /*compaction_filter*/, false /*bottommost_level*/, + 2 /*earliest_write_conflict_snapshot*/); +} + // Compaction filter should keep uncommitted key as-is, and // * Convert the latest velue to deletion, and/or // * if latest value is a merge, apply filter to all suequent merges.