MB-23906: Implement delete-with-value with store() instead of delete() 19/76119/17
authorDave Rigby <daver@couchbase.com>
Wed, 29 Mar 2017 15:15:03 +0000 (16:15 +0100)
committerDave Rigby <daver@couchbase.com>
Wed, 19 Apr 2017 07:35:14 +0000 (07:35 +0000)
commitbc90d68840b12e83f69e6b0e6b142fcc55033195
tree2aae53c358c7d3f05b7bf3b309647836038d3742
parent24a96c7df25d4169b0f657c0c790e4d79e46dec0
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>
src/ep_engine.cc
src/ep_engine.h
src/kv_bucket.cc
src/kv_bucket.h
src/kv_bucket_iface.h
src/vbucket.cc
src/vbucket.h
tests/module_tests/evp_store_rollback_test.cc
tests/module_tests/kv_bucket_test.cc
tests/module_tests/stats_test.cc
tests/module_tests/vbucket_test.cc