From bc90d68840b12e83f69e6b0e6b142fcc55033195 Mon Sep 17 00:00:00 2001 From: Dave Rigby Date: Wed, 29 Mar 2017 16:15:03 +0100 Subject: [PATCH] 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 Reviewed-by: Manu Dhundi --- src/ep_engine.cc | 45 ++------------ src/ep_engine.h | 5 -- src/kv_bucket.cc | 2 - src/kv_bucket.h | 1 - src/kv_bucket_iface.h | 3 - src/vbucket.cc | 89 ++++++++++----------------- src/vbucket.h | 4 -- tests/module_tests/evp_store_rollback_test.cc | 1 - tests/module_tests/kv_bucket_test.cc | 3 - tests/module_tests/stats_test.cc | 1 - tests/module_tests/vbucket_test.cc | 15 +---- 11 files changed, 42 insertions(+), 127 deletions(-) diff --git a/src/ep_engine.cc b/src/ep_engine.cc index d82beda..eb252c4 100644 --- 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(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(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 --- 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 --- 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 --- 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 --- 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 --- 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 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 --- 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 --- 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 --- 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 --- 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 --- 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()); -- 1.9.1