[BP] MB-29531: Replace revSeqno with a 48-bit counter 00/93800/4 watson v4.6.5
authorJim Walker <jim@couchbase.com>
Sun, 6 May 2018 19:29:12 +0000 (20:29 +0100)
committerDave Rigby <daver@couchbase.com>
Tue, 8 May 2018 08:15:10 +0000 (08:15 +0000)
Prevent a value too large to be stored in couchstore
from being placed into Item/StoredValue and also the
_local document (via vbucket_state).

Change-Id: I6de783dc2374c2634f1a729e4ca5fa2bc35dda40
Reviewed-on: http://review.couchbase.org/93800
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
src/ep.cc
src/item.h
src/kvstore.h
src/stored-value.h
tests/ep_test_apis.cc
tests/ep_test_apis.h
tests/ep_testsuite.cc
tests/ep_testsuite_xdcr.cc

index 68803ba..27a668f 100644 (file)
--- a/src/ep.cc
+++ b/src/ep.cc
@@ -3201,7 +3201,7 @@ int EventuallyPersistentStore::flushVBucket(uint16_t vbid) {
                     vbstate.maxCas = std::max(vbstate.maxCas, item->getCas());
                     if (item->isDeleted()) {
                         vbstate.maxDeletedSeqno =
-                                std::max(vbstate.maxDeletedSeqno,
+                                std::max(uint64_t(vbstate.maxDeletedSeqno),
                                          item->getRevSeqno());
                     }
                     ++stats.flusher_todo;
index 964b875..3c8c9d3 100644 (file)
@@ -21,6 +21,7 @@
 #include "config.h"
 
 #include <memcached/engine.h>
+#include <platform/n_byte_integer.h>
 #include <stdio.h>
 #include <string.h>
 
@@ -346,7 +347,7 @@ public:
     }
 
     uint64_t cas;
-    uint64_t revSeqno;
+    cb::uint48_t revSeqno;
     uint32_t flags;
     time_t exptime;
 };
index 364aa11..09fe00d 100644 (file)
@@ -189,7 +189,7 @@ struct vbucket_state {
 
     vbucket_state_t state;
     uint64_t checkpointId;
-    uint64_t maxDeletedSeqno;
+    cb::uint48_t maxDeletedSeqno;
     int64_t highSeqno;
     uint64_t purgeSeqno;
     uint64_t lastSnapStart;
index 8ecebf7..688d05a 100644 (file)
@@ -25,6 +25,7 @@
 
 #include "daemon/buffer.h"
 #include <platform/cb_malloc.h>
+#include <platform/n_byte_integer.h>
 
 // Forward declaration for StoredValue
 class HashTable;
@@ -544,11 +545,11 @@ private:
     value_t            value;          // 8 bytes
     StoredValue        *next;          // 8 bytes
     uint64_t           cas;            //!< CAS identifier.
-    uint64_t           revSeqno;       //!< Revision id sequence number
     int64_t            bySeqno;        //!< By sequence id number
     rel_time_t         lock_expiry;    //!< getl lock expiration
     uint32_t           exptime;        //!< Expiration time of this item.
     uint32_t           flags;          // 4 bytes
+    cb::uint48_t revSeqno; //!< Revision id sequence number
     bool               _isDirty  :  1; // 1 bit
     bool               deleted   :  1;
     bool               newCacheItem : 1;
index 9cc9c92..ec23215 100644 (file)
@@ -175,8 +175,9 @@ bool add_response_get_meta(const void *key, uint16_t keylen, const void *ext,
         memcpy(&last_meta.flags, ext_bytes + 4, 4);
         memcpy(&last_meta.exptime, ext_bytes + 8, 4);
         last_meta.exptime = ntohl(last_meta.exptime);
-        memcpy(&last_meta.revSeqno, ext_bytes + 12, 8);
-        last_meta.revSeqno = ntohll(last_meta.revSeqno);
+        uint64_t revId = 0;
+        memcpy(&revId, ext_bytes + 12, 8);
+        last_meta.revSeqno = ntohll(revId);
         last_meta.cas = cas;
         if (extlen > 20) {
             memcpy(&last_conflict_resolution_mode, ext_bytes + 20, 1);
@@ -215,8 +216,9 @@ bool add_response_ret_meta(const void *key, uint16_t keylen, const void *ext,
         memcpy(&last_meta.flags, ext_bytes, 4);
         memcpy(&last_meta.exptime, ext_bytes + 4, 4);
         last_meta.exptime = ntohl(last_meta.exptime);
-        memcpy(&last_meta.revSeqno, ext_bytes + 8, 8);
-        last_meta.revSeqno = ntohll(last_meta.revSeqno);
+        uint64_t revId = 0;
+        memcpy(&revId, ext_bytes + 8, 8);
+        last_meta.revSeqno = ntohll(revId);
         last_meta.cas = cas;
     }
     return add_response(key, keylen, ext, extlen, body, bodylen, datatype,
@@ -287,16 +289,31 @@ void encodeExt(char *buffer, uint32_t val) {
     memcpy(buffer, (char*)&val, sizeof(val));
 }
 
-void encodeWithMetaExt(char *buffer, ItemMetaData *meta) {
+void encodeWithMetaExt(char* buffer,
+                       uint64_t cas,
+                       uint64_t revSeqno,
+                       uint32_t flags,
+                       uint32_t exp) {
+    memcpy(buffer, (char*)&flags, sizeof(flags));
+    memcpy(buffer + 4, (char*)&exp, sizeof(exp));
+    memcpy(buffer + 8, (char*)&revSeqno, sizeof(revSeqno));
+    memcpy(buffer + 16, (char*)&cas, sizeof(cas));
+}
+
+void encodeWithMetaExt(char* buffer, RawItemMetaData* meta) {
     uint32_t flags = meta->flags;
     uint32_t exp = htonl(meta->exptime);
     uint64_t seqno = htonll(meta->revSeqno);
     uint64_t cas = htonll(meta->cas);
+    encodeWithMetaExt(buffer, cas, seqno, flags, exp);
+}
 
-    memcpy(buffer, (char*)&flags, sizeof(flags));
-    memcpy(buffer + 4, (char*)&exp, sizeof(exp));
-    memcpy(buffer + 8, (char*)&seqno, sizeof(seqno));
-    memcpy(buffer + 16, (char*)&cas, sizeof(cas));
+void encodeWithMetaExt(char* buffer, ItemMetaData* meta) {
+    uint32_t flags = meta->flags;
+    uint32_t exp = htonl(meta->exptime);
+    uint64_t seqno = htonll(meta->revSeqno);
+    uint64_t cas = htonll(meta->cas);
+    encodeWithMetaExt(buffer, cas, seqno, flags, exp);
 }
 
 protocol_binary_request_header* createPacket(uint8_t opcode,
@@ -386,6 +403,32 @@ void del_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
                    ItemMetaData *itemMeta, uint64_t cas_for_delete,
                    uint32_t options, const void *cookie,
                    const std::vector<char>& nmeta) {
+    RawItemMetaData meta{itemMeta->cas,
+                         itemMeta->revSeqno,
+                         itemMeta->flags,
+                         itemMeta->exptime};
+    del_with_meta(h,
+                  h1,
+                  key,
+                  keylen,
+                  vb,
+                  &meta,
+                  cas_for_delete,
+                  options,
+                  cookie,
+                  nmeta);
+}
+
+void del_with_meta(ENGINE_HANDLE* h,
+                   ENGINE_HANDLE_V1* h1,
+                   const char* key,
+                   const size_t keylen,
+                   const uint32_t vb,
+                   RawItemMetaData* itemMeta,
+                   uint64_t cas_for_delete,
+                   uint32_t options,
+                   const void* cookie,
+                   const std::vector<char>& nmeta) {
     int blen = 24;
     std::unique_ptr<char[]> ext(new char[30]);
     std::unique_ptr<ExtendedMetaData> emd;
index 6015e45..0d74ca1 100644 (file)
@@ -150,6 +150,28 @@ private:
     useconds_t totalSleepTime;
 };
 
+/**
+ * Raw meta-data allowing 64-bit rev-seqno
+ */
+class RawItemMetaData {
+public:
+    RawItemMetaData()
+        : cas(0), revSeqno(DEFAULT_REV_SEQ_NUM), flags(0), exptime(0) {
+    }
+
+    RawItemMetaData(uint64_t c, uint64_t s, uint32_t f, time_t e)
+        : cas(c),
+          revSeqno(s == 0 ? DEFAULT_REV_SEQ_NUM : s),
+          flags(f),
+          exptime(e) {
+    }
+
+    uint64_t cas;
+    uint64_t revSeqno;
+    uint32_t flags;
+    time_t exptime;
+};
+
 void decayingSleep(useconds_t *sleepTime);
 
 
@@ -385,6 +407,18 @@ void del_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
                    uint32_t options = 0,  const void *cookie = nullptr,
                    const std::vector<char>& nmeta = {});
 
+// This version takes a RawItemMetaData allowing for 64-bit rev-seqno tests
+void del_with_meta(ENGINE_HANDLE* h,
+                   ENGINE_HANDLE_V1* h1,
+                   const char* key,
+                   const size_t keylen,
+                   const uint32_t vb,
+                   RawItemMetaData* itemMeta,
+                   uint64_t cas_for_delete = 0,
+                   uint32_t options = 0,
+                   const void* cookie = nullptr,
+                   const std::vector<char>& nmeta = {});
+
 void return_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
                  const size_t keylen, const char *val, const size_t vallen,
                  const uint32_t vb, const uint64_t cas, const uint32_t flags,
index 221062e..f893b7b 100644 (file)
@@ -3988,7 +3988,7 @@ static enum test_result test_revid(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1)
 
         checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(),
                 "Expected success");
-        checkeq(ii, last_meta.revSeqno, "Unexpected sequence number");
+        checkeq(ii, uint64_t(last_meta.revSeqno), "Unexpected sequence number");
     }
 
     return SUCCESS;
@@ -4723,7 +4723,7 @@ static enum test_result test_set_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 0, "Invalid result for flags");
     check(last_meta.exptime == 0, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 1, "Invalid result for seqno");
+    check(last_meta.revSeqno == 1ull, "Invalid result for seqno");
 
     // Check that set with correct cas succeeds
     set_ret_meta(h, h1, "key", 3, "value", 5, 0, last_meta.cas, 10, 1735689600);
@@ -4735,7 +4735,7 @@ static enum test_result test_set_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 10, "Invalid result for flags");
     check(last_meta.exptime == 1735689600, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 2, "Invalid result for seqno");
+    check(last_meta.revSeqno == 2ull, "Invalid result for seqno");
 
     // Check that updating an item with no cas succeeds
     set_ret_meta(h, h1, "key", 3, "value", 5, 0, 0, 5, 0);
@@ -4747,7 +4747,7 @@ static enum test_result test_set_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 5, "Invalid result for flags");
     check(last_meta.exptime == 0, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 3, "Invalid result for seqno");
+    check(last_meta.revSeqno == 3ull, "Invalid result for seqno");
 
     // Check that updating an item with the wrong cas fails
     set_ret_meta(h, h1, "key", 3, "value", 5, 0, last_meta.cas + 1, 5, 0);
@@ -4821,7 +4821,7 @@ static enum test_result test_add_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 0, "Invalid result for flags");
     check(last_meta.exptime == 0, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 1, "Invalid result for seqno");
+    check(last_meta.revSeqno == 1ull, "Invalid result for seqno");
 
     // Check that re-adding a key fails
     add_ret_meta(h, h1, "key", 3, "value", 5, 0, 0, 0, 0);
@@ -4838,7 +4838,7 @@ static enum test_result test_add_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 10, "Invalid result for flags");
     check(last_meta.exptime == 1735689600, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 1, "Invalid result for seqno");
+    check(last_meta.revSeqno == 1ull, "Invalid result for seqno");
 
     return SUCCESS;
 }
@@ -4908,7 +4908,7 @@ static enum test_result test_del_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 0, "Invalid result for flags");
     check(last_meta.exptime == 0, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 1, "Invalid result for seqno");
+    check(last_meta.revSeqno == 1ull, "Invalid result for seqno");
 
     del_ret_meta(h, h1, "key", 3, 0, 0);
     checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(),
@@ -4919,7 +4919,7 @@ static enum test_result test_del_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 0, "Invalid result for flags");
     check(last_meta.exptime == 0, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 2, "Invalid result for seqno");
+    check(last_meta.revSeqno == 2ull, "Invalid result for seqno");
 
     // Check that deleting a key with a cas succeeds.
     add_ret_meta(h, h1, "key", 3, "value", 5, 0, 0, 10, 1735689600);
@@ -4929,7 +4929,7 @@ static enum test_result test_del_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 10, "Invalid result for flags");
     check(last_meta.exptime == 1735689600, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 3, "Invalid result for seqno");
+    check(last_meta.revSeqno == 3ull, "Invalid result for seqno");
 
     del_ret_meta(h, h1, "key", 3, 0, last_meta.cas);
     checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(),
@@ -4940,7 +4940,7 @@ static enum test_result test_del_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 10, "Invalid result for flags");
     check(last_meta.exptime == 1735689600, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 4, "Invalid result for seqno");
+    check(last_meta.revSeqno == 4ull, "Invalid result for seqno");
 
     // Check that deleting a key with the wrong cas fails
     add_ret_meta(h, h1, "key", 3, "value", 5, 0, 0, 0, 0);
@@ -4950,7 +4950,7 @@ static enum test_result test_del_ret_meta(ENGINE_HANDLE *h,
     check(last_meta.flags == 0, "Invalid result for flags");
     check(last_meta.exptime == 0, "Invalid result for expiration");
     check(last_meta.cas != 0, "Invalid result for cas");
-    check(last_meta.revSeqno == 5, "Invalid result for seqno");
+    check(last_meta.revSeqno == 5ull, "Invalid result for seqno");
 
     del_ret_meta(h, h1, "key", 3, 0, last_meta.cas + 1);
     checkeq(PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, last_status.load(),
index 90123da..cb58d9c 100644 (file)
@@ -836,7 +836,7 @@ static enum test_result test_set_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h
     // get metadata again to verify that set with meta was successful
     check(get_meta(h, h1, key), "Expected to get meta");
     checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(), "Expected success");
-    check(last_meta.revSeqno == 10, "Expected seqno to match");
+    check(last_meta.revSeqno == 10ull, "Expected seqno to match");
     check(last_meta.cas == 0xdeadbeef, "Expected cas to match");
     check(last_meta.flags == 0xdeadbeef, "Expected flags to match");
 
@@ -889,7 +889,7 @@ static enum test_result test_set_with_meta_by_force(ENGINE_HANDLE *h,
     // get metadata again to verify that the warmup loads an item correctly.
     check(get_meta(h, h1, key), "Expected to get meta");
     checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(), "Expected success");
-    check(last_meta.revSeqno == 10, "Expected seqno to match");
+    check(last_meta.revSeqno == 10ull, "Expected seqno to match");
     check(last_meta.cas == 0xdeadbeef, "Expected cas to match");
     check(last_meta.flags == 0xdeadbeef, "Expected flags to match");
 
@@ -1203,7 +1203,7 @@ static enum test_result test_exp_persisted_set_del(ENGINE_HANDLE *h,
 
     check(get_meta(h, h1, "key3"), "Expected to get meta");
     checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(), "Expected success");
-    check(last_meta.revSeqno == 4, "Expected seqno to match");
+    check(last_meta.revSeqno == 4ull, "Expected seqno to match");
     check(last_meta.cas != 3, "Expected cas to be different");
     check(last_meta.flags == 0, "Expected flags to match");
 
@@ -2047,6 +2047,60 @@ static enum test_result test_cas_options_and_nmeta(ENGINE_HANDLE *h,
     return SUCCESS;
 }
 
+// A delete_with_meta with a large seqno (upper 16-bits dirty) should trigger
+// a conflict if evicted and a second identical delete_with_meta occurs
+static enum test_result test_MB29119(ENGINE_HANDLE* h, ENGINE_HANDLE_V1* h1) {
+    const char* key1 = "delete_with_meta_key1";
+    const size_t keylen = strlen(key1);
+    RawItemMetaData itemMeta;
+
+    // Overflow seqno from 48-bits
+    itemMeta.revSeqno = 0x0080a80000000001;
+    itemMeta.cas = 0xdeadbeef;
+    itemMeta.exptime = 0;
+    itemMeta.flags = 0xdeadbeef;
+
+    const void* cookie = testHarness.create_cookie();
+
+    // delete an item with meta data
+    del_with_meta(h,
+                  h1,
+                  key1,
+                  keylen,
+                  0,
+                  &itemMeta,
+                  0 /*cas*/,
+                  0 /*options*/,
+                  cookie);
+
+    checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS,
+            last_status.load(),
+            "Expected success");
+
+    wait_for_flusher_to_settle(h, h1);
+    // evict the key
+    evict_key(h, h1, key1);
+
+    // Same key
+    del_with_meta(h,
+                  h1,
+                  key1,
+                  keylen,
+                  0,
+                  &itemMeta,
+                  0 /*cas*/,
+                  0 /*options*/,
+                  cookie);
+
+    // Conflict resolution must stop the second delete
+    checkeq(PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS,
+            last_status.load(),
+            "Expected EEXISTS");
+
+    testHarness.destroy_cookie(cookie);
+    return SUCCESS;
+}
+
 // Test manifest //////////////////////////////////////////////////////////////
 
 const char *default_dbname = "./ep_testsuite_xdcr";
@@ -2159,5 +2213,13 @@ BaseTestCase testsuite_testcases[] = {
                  "conflict_resolution_type=seqno",
                  prepare, cleanup),
 
+        TestCase("MB29119",
+                 test_MB29119,
+                 test_setup,
+                 teardown,
+                 "item_eviction_policy=full_eviction",
+                 prepare,
+                 cleanup),
+
         TestCase(NULL, NULL, NULL, NULL, NULL, prepare, cleanup)
 };