RocksDB: Persist tombstones in RocksDBKVStore

We cannot map a KVStore level `del` to a RocksDB `Delete` because the
deleted item actually needs to remain as a tombstone.
This is resolved by implementing RocksDBKVStore::del as a RocksDB `Put`.

NB: Until expiration is implemented, tombstones will never be purged.

Change-Id: I2cff6f21cde3625f9bd36f06828702432b56b37d
Reviewed-on: http://review.couchbase.org/82321
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: jameseh96

engines/ep/src/rocksdb-kvstore/rocksdb-kvstore.cc

@@ -99,7 +99,11 @@ void RocksDBKVStore::rollback() {
 
 void RocksDBKVStore::adjustValBuffer(const size_t to) {
     // Save room for the flags, exp, etc...
-    size_t needed((sizeof(uint32_t) * 2) + to);
+    size_t needed(sizeof(ItemMetaData) +
+                  /* deleted flag */ sizeof(uint8_t) +
+                  /* bySeqno */ sizeof(uint64_t) +
+                  /* datatype */ sizeof(uint8_t) +
+                  /* value len */ sizeof(uint32_t) + to);
 
     if (valBuffer == NULL || valSize < needed) {
         void* buf = realloc(valBuffer, needed);
@@ -118,19 +122,26 @@ std::vector<vbucket_state*> RocksDBKVStore::listPersistedVbuckets() {
 }
 
 void RocksDBKVStore::set(const Item& itm, Callback<mutation_result>& cb) {
-    // TODO RDB: Consider using SliceParts to avoid copying if
-    // possible.
-    auto k = mkKeyStr(itm.getVBucketId(), itm.getKey());
-    auto v = mkValSlice(itm);
-
-    batch->Put(k, v);
+    storeItem(itm);
 
     // TODO RDB: This callback should not really be called until
     // after the batch is committed.
     std::pair<int, bool> p(1, true);
     cb.callback(p);
 }
 
+void RocksDBKVStore::storeItem(const Item& itm) {
+    // TODO RDB: Consider using SliceParts to avoid copying if
+    // possible.
+    uint16_t vbid = itm.getVBucketId();
+    auto k = mkKeyStr(vbid, itm.getKey());
+    auto v = mkValSlice(itm);
+
+    // TODO RDB: check status.
+    rocksdb::Status s = batch->Put(k, v);
+    cb_assert(s.ok());
+}
+
 GetValue RocksDBKVStore::get(const DocKey& key, uint16_t vb, bool fetchDelete) {
     return getWithHeader(nullptr, key, vb, GetMetaOnly::No, fetchDelete);
 }
@@ -180,8 +191,10 @@ void RocksDBKVStore::reset(uint16_t vbucketId) {
 }
 
 void RocksDBKVStore::del(const Item& itm, Callback<int>& cb) {
-    auto k(mkKeyStr(itm.getVBucketId(), itm.getKey()));
-    batch->Delete(k);
+    // TODO RDB: Deleted items remain as tombstones, but are not yet
+    // expired - they will accumuate forever.
+    storeItem(itm);
+
     int rv(1);
     cb.callback(rv);
 }
@@ -262,16 +275,22 @@ rocksdb::Slice RocksDBKVStore::mkValSlice(const Item& item) {
     //    uint64_t           revSeqno     ] ItemMetaData
     //    uint32_t           flags        ]
     //    uint32_t           exptime      ]
+    // TODO RDB: Wasting a whole byte for deleted
+    //    uint8_t            deleted
     //    uint64_t           bySeqno
     //    uint8_t            datatype
     //    uint32_t           value_len
     //    uint8_t[value_len] value
-    //
-    adjustValBuffer(item.size());
+
+    adjustValBuffer(item.getNBytes());
     char* dest = valBuffer;
     std::memcpy(dest, &item.getMetaData(), sizeof(ItemMetaData));
     dest += sizeof(ItemMetaData);
 
+    uint8_t deleted = item.isDeleted();
+    std::memcpy(dest, &deleted, sizeof(deleted));
+    dest += sizeof(deleted);
+
     const int64_t bySeqno{item.getBySeqno()};
     std::memcpy(dest, &bySeqno, sizeof(bySeqno));
     dest += sizeof(uint64_t);
@@ -283,7 +302,9 @@ rocksdb::Slice RocksDBKVStore::mkValSlice(const Item& item) {
     const uint32_t valueLen = item.getNBytes();
     std::memcpy(dest, &valueLen, sizeof(valueLen));
     dest += sizeof(valueLen);
-    std::memcpy(dest, item.getValue()->getData(), valueLen);
+    if (valueLen) {
+        std::memcpy(dest, item.getValue()->getData(), valueLen);
+    }
     dest += valueLen;
 
     return rocksdb::Slice(valBuffer, dest - valBuffer);
@@ -302,6 +323,10 @@ std::unique_ptr<Item> RocksDBKVStore::grokValSlice(uint16_t vb,
     std::memcpy(&meta, src, sizeof(meta));
     src += sizeof(meta);
 
+    uint8_t deleted;
+    std::memcpy(&deleted, src, sizeof(deleted));
+    src += sizeof(deleted);
+
     int64_t bySeqno;
     std::memcpy(&bySeqno, src, sizeof(bySeqno));
     src += sizeof(bySeqno);
@@ -317,17 +342,23 @@ std::unique_ptr<Item> RocksDBKVStore::grokValSlice(uint16_t vb,
     uint8_t extMeta[EXT_META_LEN];
     extMeta[0] = datatype;
 
-    return std::make_unique<Item>(key,
-                                  meta.flags,
-                                  meta.exptime,
-                                  src,
-                                  valueLen,
-                                  extMeta,
-                                  EXT_META_LEN,
-                                  meta.cas,
-                                  bySeqno,
-                                  vb,
-                                  meta.revSeqno);
+    auto item = std::make_unique<Item>(key,
+                                       meta.flags,
+                                       meta.exptime,
+                                       src,
+                                       valueLen,
+                                       extMeta,
+                                       EXT_META_LEN,
+                                       meta.cas,
+                                       bySeqno,
+                                       vb,
+                                       meta.revSeqno);
+
+    if (deleted) {
+        item->setDeleted();
+    }
+
+    return item;
 }
 
 GetValue RocksDBKVStore::makeGetValue(uint16_t vb,

engines/ep/src/rocksdb-kvstore/rocksdb-kvstore.h

@@ -282,6 +282,8 @@ class RocksDBKVStore : public KVStore {
                           const DocKey& key,
                           const std::string& value);
 
+    void storeItem(const Item& item);
+
     std::unique_ptr<rocksdb::WriteBatch> batch;
     rocksdb::WriteOptions writeOptions;
 

engines/ep/tests/ep_testsuite.cc

@@ -7420,14 +7420,8 @@ BaseTestCase testsuite_testcases[] = {
                  NULL, prepare, cleanup),
         TestCase("test observe single key", test_observe_single_key, test_setup, teardown,
                  NULL, prepare, cleanup),
-        TestCase("test observe on temp item",
-                 test_observe_temp_item,
-                 test_setup,
-                 teardown,
-                 NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
-                 cleanup),
+        TestCase("test observe on temp item", test_observe_temp_item, test_setup, teardown,
+                 NULL, prepare, cleanup),
         TestCase("test observe multi key", test_observe_multi_key, test_setup, teardown,
                  NULL, prepare, cleanup),
         TestCase("test multiple observes", test_multiple_observes, test_setup, teardown,

engines/ep/tests/ep_testsuite_basic.cc

@@ -2287,14 +2287,8 @@ BaseTestCase testsuite_testcases[] = {
                  /* TODO RDB: implement getItemCount */
                  prepare_skip_broken_under_rocks,
                  cleanup),
-        TestCase("delete with value CAS",
-                 test_delete_with_value_cas,
-                 test_setup,
-                 teardown,
-                 NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
-                 cleanup),
+        TestCase("delete with value CAS", test_delete_with_value_cas,
+                 test_setup, teardown, NULL, prepare, cleanup),
         TestCase("set/delete", test_set_delete, test_setup,
                  teardown, NULL, prepare, cleanup),
         TestCase("set/delete (invalid cas)", test_set_delete_invalid_cas,

engines/ep/tests/ep_testsuite_xdcr.cc

@@ -2522,24 +2522,12 @@ BaseTestCase testsuite_testcases[] = {
                   * isMetaOnly not return value */
                  prepare_skip_broken_under_rocks,
                  cleanup),
-        TestCase("get meta deleted",
-                 test_get_meta_deleted,
-                 test_setup,
-                 teardown,
-                 NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
-                 cleanup),
+        TestCase("get meta deleted", test_get_meta_deleted,
+                 test_setup, teardown, NULL, prepare, cleanup),
         TestCase("get meta nonexistent", test_get_meta_nonexistent,
                  test_setup, teardown, NULL, prepare, cleanup),
-        TestCase("get meta followed by get",
-                 test_get_meta_with_get,
-                 test_setup,
-                 teardown,
-                 NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
-                 cleanup),
+        TestCase("get meta followed by get", test_get_meta_with_get,
+                 test_setup, teardown, NULL, prepare, cleanup),
         TestCase("get meta followed by set",
                  test_get_meta_with_set,
                  test_setup,
@@ -2548,14 +2536,8 @@ BaseTestCase testsuite_testcases[] = {
                  /* TODO RDB: implement getItemCount */
                  prepare_skip_broken_under_rocks,
                  cleanup),
-        TestCase("get meta followed by delete",
-                 test_get_meta_with_delete,
-                 test_setup,
-                 teardown,
-                 NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
-                 cleanup),
+        TestCase("get meta followed by delete", test_get_meta_with_delete,
+                 test_setup, teardown, NULL, prepare, cleanup),
         TestCase("get meta with xattr",
                  test_get_meta_with_xattr,
                  test_setup,
@@ -2573,16 +2555,14 @@ BaseTestCase testsuite_testcases[] = {
                  test_setup,
                  teardown,
                  nullptr,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
+                 prepare,
                  cleanup),
         TestCase("delete with meta nonexistent",
                  test_delete_with_meta_nonexistent,
                  test_setup,
                  teardown,
                  nullptr,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
+                 prepare,
                  cleanup),
         TestCase("delete with meta nonexistent no temp",
                  test_delete_with_meta_nonexistent_no_temp,
@@ -2592,21 +2572,11 @@ BaseTestCase testsuite_testcases[] = {
                  prepare,
                  cleanup),
         TestCase("delete_with_meta race with concurrent delete",
-                 test_delete_with_meta_race_with_delete,
-                 test_setup,
-                 teardown,
-                 NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
-                 cleanup),
+                 test_delete_with_meta_race_with_delete, test_setup,
+                 teardown, NULL, prepare, cleanup),
         TestCase("delete_with_meta race with concurrent set",
-                 test_delete_with_meta_race_with_set,
-                 test_setup,
-                 teardown,
-                 NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
-                 cleanup),
+                 test_delete_with_meta_race_with_set, test_setup,
+                 teardown, NULL, prepare, cleanup),
         TestCase("set with meta",
                  test_set_with_meta,
                  test_setup,
@@ -2634,44 +2604,28 @@ BaseTestCase testsuite_testcases[] = {
                  prepare_skip_broken_under_rocks,
                  cleanup),
         TestCase("set_with_meta race with concurrent set",
-                 test_set_with_meta_race_with_set,
-                 test_setup,
-                 teardown,
-                 NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
-                 cleanup),
+                 test_set_with_meta_race_with_set, test_setup,
+                 teardown, NULL, prepare, cleanup),
         TestCase("set_with_meta race with concurrent delete",
-                 test_set_with_meta_race_with_delete,
-                 test_setup,
-                 teardown,
-                 NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
-                 cleanup),
-        TestCase("test set_with_meta exp persisted",
-                 test_exp_persisted_set_del,
-                 test_setup,
-                 teardown,
-                 "exp_pager_stime=3",
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_ep_bucket_skip_broken_under_rocks,  // Requires persistence
+                 test_set_with_meta_race_with_delete, test_setup,
+                 teardown, NULL, prepare, cleanup),
+        TestCase("test set_with_meta exp persisted", test_exp_persisted_set_del,
+                 test_setup, teardown, "exp_pager_stime=3",
+                 prepare_ep_bucket,  // Requires persistence
                  cleanup),
         TestCase("test del meta conflict resolution",
                  test_del_meta_conflict_resolution,
                  test_setup,
                  teardown,
                  nullptr,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
+                 prepare,
                  cleanup),
         TestCase("test add meta conflict resolution",
                  test_add_meta_conflict_resolution,
                  test_setup,
                  teardown,
                  NULL,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
+                 prepare,
                  cleanup),
         TestCase("test set meta conflict resolution",
                  test_set_meta_conflict_resolution, test_setup, teardown, NULL,
@@ -2681,8 +2635,7 @@ BaseTestCase testsuite_testcases[] = {
                  test_setup,
                  teardown,
                  "conflict_resolution_type=lww",
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_skip_broken_under_rocks,
+                 prepare,
                  cleanup),
         TestCase("test set meta lww conflict resolution",
                  test_set_meta_lww_conflict_resolution, test_setup, teardown,
@@ -2712,9 +2665,7 @@ BaseTestCase testsuite_testcases[] = {
                  test_setup,
                  teardown,
                  "exp_pager_stime=1",
-                 /* related to temp items in hash table */
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_ep_bucket_skip_broken_under_rocks,
+                 /* related to temp items in hash table */prepare_ep_bucket,
                  cleanup),
         TestCase("test get_meta with item_eviction",
                  test_getMeta_with_item_eviction, test_setup, teardown,
@@ -2757,14 +2708,8 @@ BaseTestCase testsuite_testcases[] = {
                  test_cas_options_and_nmeta, test_setup, teardown,
                  "conflict_resolution_type=seqno",
                  prepare, cleanup),
-        TestCase("getMetaData mb23905",
-                 test_get_meta_mb23905,
-                 test_setup,
-                 teardown,
-                 nullptr,
-                 /* TODO RDB: Enable after implementing persisted tombstones */
-                 prepare_ep_bucket_skip_broken_under_rocks,
-                 cleanup),
+        TestCase("getMetaData mb23905", test_get_meta_mb23905,
+                 test_setup, teardown, nullptr, prepare_ep_bucket, cleanup),
         TestCase(NULL, NULL, NULL, NULL, NULL, prepare, cleanup)
 };