diff --git a/HISTORY.md b/HISTORY.md index e2f11c5ba..4e5f4f93e 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,5 +1,7 @@ # Rocksdb Change Log ## Unreleased +### Public API Changes +* `SstFileWriter::DeleteRange()` now returns `Status::InvalidArgument` if the range's end key comes before its start key according to the user comparator. Previously the behavior was undefined. ## 8.1.0 (03/18/2023) ### Behavior changes diff --git a/db/external_sst_file_basic_test.cc b/db/external_sst_file_basic_test.cc index 51c53a41f..950595fa1 100644 --- a/db/external_sst_file_basic_test.cc +++ b/db/external_sst_file_basic_test.cc @@ -1463,6 +1463,32 @@ TEST_F(ExternalSSTFileBasicTest, AdjacentRangeDeletionTombstones) { DestroyAndRecreateExternalSSTFilesDir(); } +TEST_F(ExternalSSTFileBasicTest, RangeDeletionEndComesBeforeStart) { + Options options = CurrentOptions(); + SstFileWriter sst_file_writer(EnvOptions(), options); + + // "file.sst" + // Verify attempt to delete 300 => 200 fails. + // Then, verify attempt to delete 300 => 300 succeeds but writes nothing. + // Afterwards, verify attempt to delete 300 => 400 works normally. + std::string file = sst_files_dir_ + "file.sst"; + ASSERT_OK(sst_file_writer.Open(file)); + ASSERT_TRUE( + sst_file_writer.DeleteRange(Key(300), Key(200)).IsInvalidArgument()); + ASSERT_OK(sst_file_writer.DeleteRange(Key(300), Key(300))); + ASSERT_OK(sst_file_writer.DeleteRange(Key(300), Key(400))); + ExternalSstFileInfo file_info; + Status s = sst_file_writer.Finish(&file_info); + ASSERT_OK(s) << s.ToString(); + ASSERT_EQ(file_info.file_path, file); + ASSERT_EQ(file_info.num_entries, 0); + ASSERT_EQ(file_info.smallest_key, ""); + ASSERT_EQ(file_info.largest_key, ""); + ASSERT_EQ(file_info.num_range_del_entries, 1); + ASSERT_EQ(file_info.smallest_range_del_key, Key(300)); + ASSERT_EQ(file_info.largest_range_del_key, Key(400)); +} + TEST_P(ExternalSSTFileBasicTest, IngestFileWithBadBlockChecksum) { bool change_checksum_called = false; const auto& change_checksum = [&](void* arg) { diff --git a/include/rocksdb/sst_file_writer.h b/include/rocksdb/sst_file_writer.h index 2a5d11db3..b44633a7f 100644 --- a/include/rocksdb/sst_file_writer.h +++ b/include/rocksdb/sst_file_writer.h @@ -145,12 +145,16 @@ class SstFileWriter { // Add a range deletion tombstone to currently opened file. Such a range // deletion tombstone does NOT delete other (point) keys in the same file. + // + // REQUIRES: The comparator orders `begin_key` at or before `end_key` // REQUIRES: comparator is *not* timestamp-aware. Status DeleteRange(const Slice& begin_key, const Slice& end_key); // Add a range deletion tombstone to currently opened file. Such a range // deletion tombstone does NOT delete other (point) keys in the same file. + // // REQUIRES: begin_key and end_key are user keys without timestamp. + // REQUIRES: The comparator orders `begin_key` at or before `end_key` // REQUIRES: the timestamp's size is equal to what is expected by // the comparator. Status DeleteRange(const Slice& begin_key, const Slice& end_key, diff --git a/table/sst_file_writer.cc b/table/sst_file_writer.cc index e9f72f04f..d187b741e 100644 --- a/table/sst_file_writer.cc +++ b/table/sst_file_writer.cc @@ -134,6 +134,17 @@ struct SstFileWriter::Rep { if (!builder) { return Status::InvalidArgument("File is not opened"); } + int cmp = internal_comparator.user_comparator()->CompareWithoutTimestamp( + begin_key, end_key); + if (cmp > 0) { + // It's an empty range where endpoints appear mistaken. Don't bother + // applying it to the DB, and return an error to the user. + return Status::InvalidArgument("end key comes before start key"); + } else if (cmp == 0) { + // It's an empty range. Don't bother applying it to the DB. + return Status::OK(); + } + RangeTombstone tombstone(begin_key, end_key, 0 /* Sequence Number */); if (file_info.num_range_del_entries == 0) { file_info.smallest_range_del_key.assign(tombstone.start_key_.data(),