From 0f9dcb88b231513dd0d78544cda5034924d65226 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Tue, 18 Feb 2020 11:14:37 -0800 Subject: [PATCH] Return NotSupported from WriteBatchWithIndex::DeleteRange (#5393) Summary: As discovered in https://github.com/facebook/rocksdb/issues/5260 and https://github.com/facebook/rocksdb/issues/5392, reads on the indexed batch do not account for range tombstones. So, return `Status::NotSupported` from `WriteBatchWithIndex::DeleteRange` until we properly support it. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5393 Test Plan: added unit test Differential Revision: D19912360 Pulled By: ajkr fbshipit-source-id: 0bbfc978ea015d64516ca708fce2429abba524cb --- HISTORY.md | 1 + db/c_test.c | 17 --------------- db/db_range_del_test.cc | 12 +++++++++-- db/write_batch_test.cc | 9 -------- include/rocksdb/c.h | 4 ++++ .../utilities/write_batch_with_index.h | 14 ++++++++++--- .../java/org/rocksdb/WriteBatchWithIndex.java | 2 ++ .../org/rocksdb/WriteBatchWithIndexTest.java | 7 +------ .../write_batch_with_index.cc | 21 ------------------- 9 files changed, 29 insertions(+), 58 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 5ecb95a72..9c954b496 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -11,6 +11,7 @@ * Add DBOptions::skip_checking_sst_file_sizes_on_db_open. It disables potentially expensive checking of all sst file sizes in DB::Open(). * BlobDB now ignores trivially moved files when updating the mapping between blob files and SSTs. This should mitigate issue #6338 where out of order flush/compaction notifications could trigger an assertion with the earlier code. * Batched MultiGet() ignores IO errors while reading data blocks, causing it to potentially continue looking for a key and returning stale results. +* `WriteBatchWithIndex::DeleteRange` returns `Status::NotSupported`. Previously it returned success even though reads on the batch did not account for range tombstones. The corresponding language bindings now cannot be used. In C, that includes `rocksdb_writebatch_wi_delete_range`, `rocksdb_writebatch_wi_delete_range_cf`, `rocksdb_writebatch_wi_delete_rangev`, and `rocksdb_writebatch_wi_delete_rangev_cf`. In Java, that includes `WriteBatchWithIndex::deleteRange`. ### Performance Improvements * Perfom readahead when reading from option files. Inside DB, options.log_readahead_size will be used as the readahead size. In other cases, a default 512KB is used. diff --git a/db/c_test.c b/db/c_test.c index bee1837d0..cf2e266f9 100644 --- a/db/c_test.c +++ b/db/c_test.c @@ -835,23 +835,6 @@ int main(int argc, char** argv) { rocksdb_writebatch_wi_iterate(wbi, &pos, CheckPut, CheckDel); CheckCondition(pos == 3); rocksdb_writebatch_wi_clear(wbi); - rocksdb_writebatch_wi_put(wbi, "bar", 3, "b", 1); - rocksdb_writebatch_wi_put(wbi, "bay", 3, "d", 1); - rocksdb_writebatch_wi_delete_range(wbi, "bar", 3, "bay", 3); - rocksdb_write_writebatch_wi(db, woptions, wbi, &err); - CheckNoError(err); - CheckGet(db, roptions, "bar", NULL); - CheckGet(db, roptions, "bay", "d"); - rocksdb_writebatch_wi_clear(wbi); - const char* start_list[1] = {"bay"}; - const size_t start_sizes[1] = {3}; - const char* end_list[1] = {"baz"}; - const size_t end_sizes[1] = {3}; - rocksdb_writebatch_wi_delete_rangev(wbi, 1, start_list, start_sizes, end_list, - end_sizes); - rocksdb_write_writebatch_wi(db, woptions, wbi, &err); - CheckNoError(err); - CheckGet(db, roptions, "bay", NULL); rocksdb_writebatch_wi_destroy(wbi); } diff --git a/db/db_range_del_test.cc b/db/db_range_del_test.cc index f2953a746..93a16a9cf 100644 --- a/db/db_range_del_test.cc +++ b/db/db_range_del_test.cc @@ -5,6 +5,7 @@ #include "db/db_test_util.h" #include "port/stack_trace.h" +#include "rocksdb/utilities/write_batch_with_index.h" #include "test_util/testutil.h" #include "utilities/merge_operators.h" @@ -23,8 +24,8 @@ class DBRangeDelTest : public DBTestBase { } }; -// PlainTableFactory and NumTableFilesAtLevel() are not supported in -// ROCKSDB_LITE +// PlainTableFactory, WriteBatchWithIndex, and NumTableFilesAtLevel() are not +// supported in ROCKSDB_LITE #ifndef ROCKSDB_LITE TEST_F(DBRangeDelTest, NonBlockBasedTableNotSupported) { // TODO: figure out why MmapReads trips the iterator pinning assertion in @@ -39,6 +40,13 @@ TEST_F(DBRangeDelTest, NonBlockBasedTableNotSupported) { } } +TEST_F(DBRangeDelTest, WriteBatchWithIndexNotSupported) { + WriteBatchWithIndex indexedBatch{}; + ASSERT_TRUE(indexedBatch.DeleteRange(db_->DefaultColumnFamily(), "dr1", "dr1") + .IsNotSupported()); + ASSERT_TRUE(indexedBatch.DeleteRange("dr1", "dr1").IsNotSupported()); +} + TEST_F(DBRangeDelTest, FlushOutputHasOnlyRangeTombstones) { do { DestroyAndReopen(CurrentOptions()); diff --git a/db/write_batch_test.cc b/db/write_batch_test.cc index 869cfa8cb..a4c21cf62 100644 --- a/db/write_batch_test.cc +++ b/db/write_batch_test.cc @@ -655,7 +655,6 @@ TEST_F(WriteBatchTest, ColumnFamiliesBatchWithIndexTest) { batch.Put(&eight, Slice("eightfoo"), Slice("bar8")); batch.Delete(&eight, Slice("eightfoo")); batch.SingleDelete(&two, Slice("twofoo")); - batch.DeleteRange(&two, Slice("twofoo"), Slice("threefoo")); batch.Merge(&three, Slice("threethree"), Slice("3three")); batch.Put(&zero, Slice("foo"), Slice("bar")); batch.Merge(Slice("omom"), Slice("nom")); @@ -694,13 +693,6 @@ TEST_F(WriteBatchTest, ColumnFamiliesBatchWithIndexTest) { ASSERT_EQ(WriteType::kSingleDeleteRecord, iter->Entry().type); ASSERT_EQ("twofoo", iter->Entry().key.ToString()); - iter->Next(); - ASSERT_OK(iter->status()); - ASSERT_TRUE(iter->Valid()); - ASSERT_EQ(WriteType::kDeleteRangeRecord, iter->Entry().type); - ASSERT_EQ("twofoo", iter->Entry().key.ToString()); - ASSERT_EQ("threefoo", iter->Entry().value.ToString()); - iter->Next(); ASSERT_OK(iter->status()); ASSERT_TRUE(!iter->Valid()); @@ -751,7 +743,6 @@ TEST_F(WriteBatchTest, ColumnFamiliesBatchWithIndexTest) { "PutCF(8, eightfoo, bar8)" "DeleteCF(8, eightfoo)" "SingleDeleteCF(2, twofoo)" - "DeleteRangeCF(2, twofoo, threefoo)" "MergeCF(3, threethree, 3three)" "Put(foo, bar)" "Merge(omom, nom)", diff --git a/include/rocksdb/c.h b/include/rocksdb/c.h index 315b82ba8..f79729a61 100644 --- a/include/rocksdb/c.h +++ b/include/rocksdb/c.h @@ -592,17 +592,21 @@ extern ROCKSDB_LIBRARY_API void rocksdb_writebatch_wi_deletev( extern ROCKSDB_LIBRARY_API void rocksdb_writebatch_wi_deletev_cf( rocksdb_writebatch_wi_t* b, rocksdb_column_family_handle_t* column_family, int num_keys, const char* const* keys_list, const size_t* keys_list_sizes); +// DO NOT USE - rocksdb_writebatch_wi_delete_range is not yet supported extern ROCKSDB_LIBRARY_API void rocksdb_writebatch_wi_delete_range( rocksdb_writebatch_wi_t* b, const char* start_key, size_t start_key_len, const char* end_key, size_t end_key_len); +// DO NOT USE - rocksdb_writebatch_wi_delete_range_cf is not yet supported extern ROCKSDB_LIBRARY_API void rocksdb_writebatch_wi_delete_range_cf( rocksdb_writebatch_wi_t* b, rocksdb_column_family_handle_t* column_family, const char* start_key, size_t start_key_len, const char* end_key, size_t end_key_len); +// DO NOT USE - rocksdb_writebatch_wi_delete_rangev is not yet supported extern ROCKSDB_LIBRARY_API void rocksdb_writebatch_wi_delete_rangev( rocksdb_writebatch_wi_t* b, int num_keys, const char* const* start_keys_list, const size_t* start_keys_list_sizes, const char* const* end_keys_list, const size_t* end_keys_list_sizes); +// DO NOT USE - rocksdb_writebatch_wi_delete_rangev_cf is not yet supported extern ROCKSDB_LIBRARY_API void rocksdb_writebatch_wi_delete_rangev_cf( rocksdb_writebatch_wi_t* b, rocksdb_column_family_handle_t* column_family, int num_keys, const char* const* start_keys_list, diff --git a/include/rocksdb/utilities/write_batch_with_index.h b/include/rocksdb/utilities/write_batch_with_index.h index a0b3bac99..b9e3be15f 100644 --- a/include/rocksdb/utilities/write_batch_with_index.h +++ b/include/rocksdb/utilities/write_batch_with_index.h @@ -125,9 +125,17 @@ class WriteBatchWithIndex : public WriteBatchBase { Status SingleDelete(const Slice& key) override; using WriteBatchBase::DeleteRange; - Status DeleteRange(ColumnFamilyHandle* column_family, const Slice& begin_key, - const Slice& end_key) override; - Status DeleteRange(const Slice& begin_key, const Slice& end_key) override; + Status DeleteRange(ColumnFamilyHandle* /* column_family */, + const Slice& /* begin_key */, + const Slice& /* end_key */) override { + return Status::NotSupported( + "DeleteRange unsupported in WriteBatchWithIndex"); + } + Status DeleteRange(const Slice& /* begin_key */, + const Slice& /* end_key */) override { + return Status::NotSupported( + "DeleteRange unsupported in WriteBatchWithIndex"); + } using WriteBatchBase::PutLogData; Status PutLogData(const Slice& blob) override; diff --git a/java/src/main/java/org/rocksdb/WriteBatchWithIndex.java b/java/src/main/java/org/rocksdb/WriteBatchWithIndex.java index 57e4c2da5..3831f85ba 100644 --- a/java/src/main/java/org/rocksdb/WriteBatchWithIndex.java +++ b/java/src/main/java/org/rocksdb/WriteBatchWithIndex.java @@ -277,9 +277,11 @@ public class WriteBatchWithIndex extends AbstractWriteBatch { @Override final native void removeDirect(final long handle, final ByteBuffer key, final int keyOffset, final int keyLength, final long cfHandle) throws RocksDBException; + // DO NOT USE - `WriteBatchWithIndex::deleteRange` is not yet supported @Override final native void deleteRange(final long handle, final byte[] beginKey, final int beginKeyLen, final byte[] endKey, final int endKeyLen); + // DO NOT USE - `WriteBatchWithIndex::deleteRange` is not yet supported @Override final native void deleteRange(final long handle, final byte[] beginKey, final int beginKeyLen, final byte[] endKey, final int endKeyLen, final long cfHandle); diff --git a/java/src/test/java/org/rocksdb/WriteBatchWithIndexTest.java b/java/src/test/java/org/rocksdb/WriteBatchWithIndexTest.java index b2204ed3e..01eb652f1 100644 --- a/java/src/test/java/org/rocksdb/WriteBatchWithIndexTest.java +++ b/java/src/test/java/org/rocksdb/WriteBatchWithIndexTest.java @@ -193,9 +193,6 @@ public class WriteBatchWithIndexTest { // add a single deletion record wbwi.singleDelete(k5b); - // add a delete range record - wbwi.deleteRange(k6b, k7b); - // add a log record wbwi.putLogData(v8b); @@ -210,13 +207,11 @@ public class WriteBatchWithIndexTest { new DirectSlice(k4), DirectSlice.NONE), new WBWIRocksIterator.WriteEntry(WBWIRocksIterator.WriteType.SINGLE_DELETE, new DirectSlice(k5), DirectSlice.NONE), - new WBWIRocksIterator.WriteEntry(WBWIRocksIterator.WriteType.DELETE_RANGE, - new DirectSlice(k6), new DirectSlice(k7)), }; try (final WBWIRocksIterator it = wbwi.newIterator()) { //direct access - seek to key offsets - final int[] testOffsets = {2, 0, 3, 4, 1, 5}; + final int[] testOffsets = {2, 0, 3, 4, 1}; for (int i = 0; i < testOffsets.length; i++) { final int testOffset = testOffsets[i]; diff --git a/utilities/write_batch_with_index/write_batch_with_index.cc b/utilities/write_batch_with_index/write_batch_with_index.cc index 54fb2292c..a3ef816dd 100644 --- a/utilities/write_batch_with_index/write_batch_with_index.cc +++ b/utilities/write_batch_with_index/write_batch_with_index.cc @@ -736,27 +736,6 @@ Status WriteBatchWithIndex::SingleDelete(const Slice& key) { return s; } -Status WriteBatchWithIndex::DeleteRange(ColumnFamilyHandle* column_family, - const Slice& begin_key, - const Slice& end_key) { - rep->SetLastEntryOffset(); - auto s = rep->write_batch.DeleteRange(column_family, begin_key, end_key); - if (s.ok()) { - rep->AddOrUpdateIndex(column_family, begin_key); - } - return s; -} - -Status WriteBatchWithIndex::DeleteRange(const Slice& begin_key, - const Slice& end_key) { - rep->SetLastEntryOffset(); - auto s = rep->write_batch.DeleteRange(begin_key, end_key); - if (s.ok()) { - rep->AddOrUpdateIndex(begin_key); - } - return s; -} - Status WriteBatchWithIndex::Merge(ColumnFamilyHandle* column_family, const Slice& key, const Slice& value) { rep->SetLastEntryOffset();