MB-23906: Implement delete-with-value with store() instead of delete()

During investigation of issues with 'fully' deleting documents
(i.e. discarding their bodies) which are in state Delete-with-value,
it was found that there's two bugs which interact to allow some
transitions between Deleted states, but not others. As a result
Deletes are not always handled correctly.

Prior to Spock, the valid state transitions for StoredValues (relating
to deletion) were:

              /---\
              |   |
              |   V
              Alive -----------------\
                ^                    |
                |                    V
                \---------------- Deleted
                                 (no value)

With the advent of Deleted Bodies for system XATTRs, Deleted SVs can
optionally have a value (Blob) associated with them, which can be
changed (or removed) without going back to Alive:

              /---\
              |   |
              |   V
              Alive -----------------\
                ^                    |
                |                    V
                \---------------- Deleted <------------\
                           (with-value or no-value)    |
                                     |                 |
                                     \-----------------/

While the above transitions should be supported, StoredValue was not
fully updated to allow transitioning between the two Deleted variants.
Specifically, if an item had the `deleted` flag set, then calling
StoreValue::del() while in the Deleted-with-Value state became a no-op
- the value was not removed:

    void del(HashTable &ht) {
        if (isDeleted()) {
            return;
        }
        ...
        resetValue();  // <-- Value removed here

This means the Deleted-with-value -> Deleted-no-value transition is
broken - the deleted value was not removed.

If we want to fix this, and handle 'deleting-a-deleted-value' in
HashTable, then we need to allow the HashTable to re-delete an item
(essentially remove the early exit in StoredValue:del(). However in
doing so, it breaks *setting* the deleted value, due to a second bug
in VBucket::processSoftDelete():-

  VBucket::processSoftDelete() (which is the function which handles
  both Delete-with-value and Delete-no-value) assumes the deletion
  should have no value (it calls softDeleteStoredValue() with
  onlyMarkDeleted==false), and hence the item should have its body
  discarded. However due to the aforementioned 'bug' (early-exit) in
  StoredValue::del(), the body was kept (and Delete-with-Value ->
  Delete-with-Value transition currently works).

To resolve this, it seems simpler and cleaner to implement
Delete-with-Value as a Mutation - i.e. call down the EvpStore path
instead of EvpItemDelete. As EvpStore already passes an Item object
all the way down the call-chain to updateStoredValue(), we can simply
use the existing Item object to convey the the `del` op instead of a
`set`.

Change-Id: I3b9223da101b38e3e25aef237d0e52ee55bae623
Reviewed-on: http://review.couchbase.org/76119
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Manu Dhundi <manu@couchbase.com>

diff --git a/src/ep_engine.cc b/src/ep_engine.cc
index d82beda..eb252c4 100644 (file)
--- a/src/ep_engine.cc
+++ b/src/ep_engine.cc
@@ -228,7 +228,7 @@ static ENGINE_ERROR_CODE EvpItemDelete(ENGINE_HANDLE* handle,
         return ENGINE_EINVAL;
     }
     return acquireEngine(handle)->itemDelete(
-            cookie, key, *cas, vbucket, nullptr, nullptr, mut_info);
+            cookie, key, *cas, vbucket, nullptr, mut_info);
 }
 
 static void EvpItemRelease(ENGINE_HANDLE* handle,
@@ -309,47 +309,14 @@ static ENGINE_ERROR_CODE EvpStore(ENGINE_HANDLE* handle,
                                   uint64_t* cas,
                                   ENGINE_STORE_OPERATION operation,
                                   DocumentState document_state) {
-    ENGINE_ERROR_CODE err_code = ENGINE_SUCCESS;
     auto engine = acquireEngine(handle);
-    switch (document_state) {
-    case DocumentState::Alive: {
-        err_code = engine->store(cookie, itm, cas, operation);
-        break;
-    }
-    case DocumentState::Deleted: {
-        Item* item = static_cast<Item*>(itm);
-        ItemMetaData itm_meta;
-        mutation_descr_t mut_info;
-
-        if (!cas) {
-            LOG(EXTENSION_LOG_WARNING,
-                "EvpStore(): cas ptr passed is null for vb: %" PRIu16,
-                item->getVBucketId());
-            return ENGINE_EINVAL;
-        }
 
-        /* Set the item as deleted */
+    if (document_state == DocumentState::Deleted) {
+        Item* item = static_cast<Item*>(itm);
         item->setDeleted();
-        *cas = item->getCas();
-        err_code = engine->itemDelete(cookie,
-                                      item->getKey(),
-                                      *cas,
-                                      item->getVBucketId(),
-                                      item,
-                                      &itm_meta,
-                                      &mut_info);
-        if (err_code == ENGINE_SUCCESS) {
-            item->setBySeqno(mut_info.seqno);
-            item->setCas(itm_meta.cas);
-            item->setFlags(itm_meta.flags);
-            item->setExpTime(itm_meta.exptime);
-        }
-        break;
     }
-    default:
-        return ENGINE_ENOTSUP;
-    }
-    return err_code;
+
+    return engine->store(cookie, itm, cas, operation);
 }
 
 static ENGINE_ERROR_CODE EvpFlush(ENGINE_HANDLE* handle,
@@ -5783,7 +5750,7 @@ EventuallyPersistentEngine::returnMeta(const void* cookie,
         ItemMetaData itm_meta;
         DocKey key(keyPtr, keylen, docNamespace);
         ret = kvBucket->deleteItem(
-                key, cas, vbucket, cookie, nullptr, &itm_meta, nullptr);
+                key, cas, vbucket, cookie, &itm_meta, nullptr);
         if (ret == ENGINE_SUCCESS) {
             ++stats.numOpsDelRetMeta;
         }

diff --git a/src/ep_engine.h b/src/ep_engine.h
index a2cb9d1..e88de64 100644 (file)
--- a/src/ep_engine.h
+++ b/src/ep_engine.h
@@ -127,9 +127,6 @@ public:
      * @param cas CAS value of the mutation that needs to be returned
      *            back to the client
      * @param vbucket vbucket id to which the deleted key corresponds to
-     * @param itm item pointer that contains a value that needs to be
-     *            stored along with a delete. A NULL pointer indicates
-     *            that no value needs to be stored with the delete.
      * @param item_meta pointer to item meta data that needs to be
      *                  as a result the delete. A NULL pointer indicates
      *                  that no meta data needs to be returned.
@@ -143,14 +140,12 @@ public:
                                  const DocKey& key,
                                  uint64_t& cas,
                                  uint16_t vbucket,
-                                 Item* itm,
                                  ItemMetaData* item_meta,
                                  mutation_descr_t* mut_info) {
         ENGINE_ERROR_CODE ret = kvBucket->deleteItem(key,
                                                      cas,
                                                      vbucket,
                                                      cookie,
-                                                     itm,
                                                      item_meta,
                                                      mut_info);
 

diff --git a/src/kv_bucket.cc b/src/kv_bucket.cc
index 5e29ccc..9d6608e 100644 (file)
--- a/src/kv_bucket.cc
+++ b/src/kv_bucket.cc
@@ -1704,7 +1704,6 @@ ENGINE_ERROR_CODE KVBucket::deleteItem(const DocKey& key,
                                        uint64_t& cas,
                                        uint16_t vbucket,
                                        const void* cookie,
-                                       Item* itm,
                                        ItemMetaData* itemMeta,
                                        mutation_descr_t* mutInfo) {
     VBucketPtr vb = getVBucket(vbucket);
@@ -1729,7 +1728,6 @@ ENGINE_ERROR_CODE KVBucket::deleteItem(const DocKey& key,
                           cookie,
                           engine,
                           bgFetchDelay,
-                          itm,
                           itemMeta,
                           mutInfo);
 }

diff --git a/src/kv_bucket.h b/src/kv_bucket.h
index 92a4b1d..fd75837 100644 (file)
--- a/src/kv_bucket.h
+++ b/src/kv_bucket.h
@@ -240,7 +240,6 @@ public:
                                  uint64_t& cas,
                                  uint16_t vbucket,
                                  const void* cookie,
-                                 Item* itm,
                                  ItemMetaData* itemMeta,
                                  mutation_descr_t* mutInfo);
 

diff --git a/src/kv_bucket_iface.h b/src/kv_bucket_iface.h
index 27585a2..21e24c0 100644 (file)
--- a/src/kv_bucket_iface.h
+++ b/src/kv_bucket_iface.h
@@ -295,8 +295,6 @@ public:
      * @param[in, out] cas the CAS ID for a CASed delete (0 to override)
      * @param vbucket the vbucket for the key
      * @param cookie the cookie representing the client
-     * @param itm item holding a deleted value. A NULL value is passed
-                  if an empty body is to be used for deletion.
      * @param[out] itemMeta the pointer to the metadata memory.
      * @param[out] mutInfo mutation information
      *
@@ -306,7 +304,6 @@ public:
                                          uint64_t& cas,
                                          uint16_t vbucket,
                                          const void* cookie,
-                                         Item* itm,
                                          ItemMetaData* itemMeta,
                                          mutation_descr_t* mutInfo) = 0;
 

diff --git a/src/vbucket.cc b/src/vbucket.cc
index 874d3a4..33c48cb 100644 (file)
--- a/src/vbucket.cc
+++ b/src/vbucket.cc
@@ -1035,69 +1035,37 @@ ENGINE_ERROR_CODE VBucket::deleteItem(const DocKey& key,
                                       const void* cookie,
                                       EventuallyPersistentEngine& engine,
                                       const int bgFetchDelay,
-                                      Item* itm,
                                       ItemMetaData* itemMeta,
                                       mutation_descr_t* mutInfo) {
     auto hbl = ht.getLockedBucket(key);
     StoredValue* v = ht.unlocked_find(
             key, hbl.getBucketNum(), WantsDeleted::Yes, TrackReference::No);
 
-    if (!itm) {
-        if (!v || v->isDeleted() || v->isTempItem()) {
-            if (eviction == VALUE_ONLY) {
-                return ENGINE_KEY_ENOENT;
-            } else { // Full eviction.
-                if (!v) { // Item might be evicted from cache.
-                    if (maybeKeyExistsInFilter(key)) {
-                        return addTempItemAndBGFetch(
-                                hbl, key, cookie, engine, bgFetchDelay, true);
-                    } else {
-                        // As bloomfilter predicted that item surely doesn't
-                        // exist on disk, return ENOENT for deleteItem().
-                        return ENGINE_KEY_ENOENT;
-                    }
-                } else if (v->isTempInitialItem()) {
-                    hbl.getHTLock().unlock();
-                    bgFetch(key, cookie, engine, bgFetchDelay, true);
-                    return ENGINE_EWOULDBLOCK;
-                } else { // Non-existent or deleted key.
-                    if (v->isTempNonExistentItem() || v->isTempDeletedItem()) {
-                        // Delete a temp non-existent item to ensure that
-                        // if a delete were issued over an item that doesn't
-                        // exist, then we don't preserve a temp item.
-                        deleteStoredValue(hbl, *v);
-                    }
+    if (!v || v->isDeleted() || v->isTempItem()) {
+        if (eviction == VALUE_ONLY) {
+            return ENGINE_KEY_ENOENT;
+        } else { // Full eviction.
+            if (!v) { // Item might be evicted from cache.
+                if (maybeKeyExistsInFilter(key)) {
+                    return addTempItemAndBGFetch(
+                            hbl, key, cookie, engine, bgFetchDelay, true);
+                } else {
+                    // As bloomfilter predicted that item surely doesn't
+                    // exist on disk, return ENOENT for deleteItem().
                     return ENGINE_KEY_ENOENT;
                 }
-            }
-        }
-    } else {
-        /* if an item containing a deleted value is present, set the value
-         * as part of the stored value so that a value is set as part of the
-         * delete.
-         */
-        if (v) {
-            if (cas != 0 && cas != v->getCas()) {
-                return ENGINE_KEY_EEXISTS;
-            }
-            itm->setRevSeqno(v->getRevSeqno());
-            v->setValue(*itm, ht);
-        } else {
-            /* retrieve the item, if it is present in disk, for CAS comparison */
-            if (maybeKeyExistsInFilter(key)) {
-                return addTempItemAndBGFetch(
-                                hbl, key, cookie, engine, bgFetchDelay, true);
-            } else {
-                AddStatus rv = addTempStoredValue(hbl, key);
-                if (rv == AddStatus::NoMem) {
-                    return ENGINE_ENOMEM;
+            } else if (v->isTempInitialItem()) {
+                hbl.getHTLock().unlock();
+                bgFetch(key, cookie, engine, bgFetchDelay, true);
+                return ENGINE_EWOULDBLOCK;
+            } else { // Non-existent or deleted key.
+                if (v->isTempNonExistentItem() || v->isTempDeletedItem()) {
+                    // Delete a temp non-existent item to ensure that
+                    // if a delete were issued over an item that doesn't
+                    // exist, then we don't preserve a temp item.
+                    deleteStoredValue(hbl, *v);
                 }
-                v = ht.unlocked_find(key,
-                                     hbl.getBucketNum(),
-                                     WantsDeleted::Yes,
-                                     TrackReference::No);
-                v->setValue(*itm, ht);
-                /* Due to the above setValue() v is no longer a temp stored value*/
+                return ENGINE_KEY_ENOENT;
             }
         }
     }
@@ -1982,10 +1950,19 @@ std::pair<MutationStatus, VBNotifyCtx> VBucket::processSet(
             /* allow operation*/
             v->unlock();
         } else if (cas && cas != v->getCas()) {
-            if (v->isTempDeletedItem() || v->isTempNonExistentItem() ||
-                v->isDeleted()) {
+            if (v->isTempNonExistentItem()) {
+                // This is a temporary item which marks a key as non-existent;
+                // therefore specifying a non-matching CAS should be exposed
+                // as item not existing.
+                return {MutationStatus::NotFound, VBNotifyCtx()};
+            }
+            if ((v->isTempDeletedItem() || v->isDeleted()) && !itm.isDeleted()) {
+                // Existing item is deleted, and we are not replacing it with
+                // a (different) deleted value - return not existing.
                 return {MutationStatus::NotFound, VBNotifyCtx()};
             }
+            // None of the above special cases; the existing item cannot be
+            // modified with the specified CAS.
             return {MutationStatus::InvalidCas, VBNotifyCtx()};
         }
         if (!hasMetaData) {

diff --git a/src/vbucket.h b/src/vbucket.h
index ec201f2..af9b310 100644 (file)
--- a/src/vbucket.h
+++ b/src/vbucket.h
@@ -805,9 +805,6 @@ public:
      * @param cookie the cookie representing the client to store the item
      * @param engine Reference to ep engine
      * @param bgFetchDelay
-     * @param itm item pointer that contains a value that needs to be
-     *            stored along with a delete. A NULL pointer indicates
-     *            that no value needs to be stored with the delete.
      * @param[out] itemMeta pointer to item meta data that needs to be returned
      *                      as a result the delete. A NULL pointer indicates
      *                      that no meta data needs to be returned.
@@ -822,7 +819,6 @@ public:
                                  const void* cookie,
                                  EventuallyPersistentEngine& engine,
                                  int bgFetchDelay,
-                                 Item* itm,
                                  ItemMetaData* itemMeta,
                                  mutation_descr_t* mutInfo);
 

diff --git a/tests/module_tests/evp_store_rollback_test.cc b/tests/module_tests/evp_store_rollback_test.cc
index 06d4aa7..6d207bb 100644 (file)
--- a/tests/module_tests/evp_store_rollback_test.cc
+++ b/tests/module_tests/evp_store_rollback_test.cc
@@ -83,7 +83,6 @@ protected:
                                     cas,
                                     vbid,
                                     /*cookie*/ nullptr,
-                                    /*Item*/ nullptr,
                                     /*itemMeta*/ nullptr,
                                     /*mutation_descr_t*/ nullptr));
         if (flush_before_rollback) {

diff --git a/tests/module_tests/kv_bucket_test.cc b/tests/module_tests/kv_bucket_test.cc
index 4d64a68..6ede796 100644 (file)
--- a/tests/module_tests/kv_bucket_test.cc
+++ b/tests/module_tests/kv_bucket_test.cc
@@ -160,7 +160,6 @@ void KVBucketTest::delete_item(uint16_t vbid, const StoredDocKey& key) {
                                 cas,
                                 vbid,
                                 cookie,
-                                /*Item*/ nullptr,
                                 /*itemMeta*/ nullptr,
                                 /*mutation_descr_t*/ nullptr));
 }
@@ -442,7 +441,6 @@ TEST_P(KVBucketParamTest, SetCASDeleted) {
                                 cas,
                                 vbid,
                                 cookie,
-                      /*Item*/ nullptr,
                       /*itemMeta*/ nullptr,
                       /*mutation_descr_t*/ nullptr));
 
@@ -582,7 +580,6 @@ TEST_P(KVBucketParamTest, mb22824) {
                                 cas,
                                 vbid,
                                 cookie,
-                                /*Item*/ nullptr,
                                 &itemMeta2,
                                 /*mutation_descr_t*/ nullptr));
 

diff --git a/tests/module_tests/stats_test.cc b/tests/module_tests/stats_test.cc
index af6030a..96a63d7 100644 (file)
--- a/tests/module_tests/stats_test.cc
+++ b/tests/module_tests/stats_test.cc
@@ -198,7 +198,6 @@ TEST_P(DatatypeStatTest, datatypeDeletion) {
                       0,
                       cookie,
                       nullptr,
-                      nullptr,
                       nullptr);
     vals = get_stat(nullptr);
     EXPECT_EQ(0, std::stoi(vals["ep_active_datatype_json,xattr"]));

diff --git a/tests/module_tests/vbucket_test.cc b/tests/module_tests/vbucket_test.cc
index 5068eee..9d92949 100644 (file)
--- a/tests/module_tests/vbucket_test.cc
+++ b/tests/module_tests/vbucket_test.cc
@@ -333,11 +333,8 @@ TEST_P(VBucketTest, unlockedSoftDeleteWithValue) {
     auto prev_revseqno = v->getRevSeqno();
     deleted_item.setRevSeqno(prev_revseqno);
 
-    // Set a new deleted value
-    v->setValue(deleted_item, this->vbucket->ht);
-
     EXPECT_EQ(MutationStatus::WasDirty,
-              this->public_processSoftDelete(v->getKey(), v, 0));
+              this->public_processSet(deleted_item, 0));
     verifyValue(key, "deletedvalue", TrackReference::Yes, WantsDeleted::Yes);
     EXPECT_EQ(prev_revseqno + 1, v->getRevSeqno());
 }
@@ -373,11 +370,8 @@ TEST_P(VBucketTest, updateDeletedItem) {
     auto prev_revseqno = v->getRevSeqno();
     deleted_item.setRevSeqno(prev_revseqno);
 
-    // Set a new deleted value
-    v->setValue(deleted_item, this->vbucket->ht);
-
     EXPECT_EQ(MutationStatus::WasDirty,
-              this->public_processSoftDelete(v->getKey(), v, 0));
+              this->public_processSet(deleted_item, 0));
     verifyValue(
                 key,
                 "deletedvalue",
@@ -393,11 +387,8 @@ TEST_P(VBucketTest, updateDeletedItem) {
     prev_revseqno = v->getRevSeqno();
     update_deleted_item.setRevSeqno(prev_revseqno);
 
-    // Set a new deleted value
-    v->setValue(update_deleted_item, this->vbucket->ht);
-
     EXPECT_EQ(MutationStatus::WasDirty,
-              this->public_processSoftDelete(v->getKey(), v, 0));
+              this->public_processSet(update_deleted_item, 0));
     verifyValue(
             key, "updatedeletedvalue", TrackReference::Yes, WantsDeleted::Yes);
     EXPECT_EQ(prev_revseqno + 1, v->getRevSeqno());