MB-23905: getWithMeta doesn't need to bgFetch for datatype

Some more code left-over from when datatype was part of the value
is now removed. getMeta only needs to perform a meta-data fetch
and have no special case for datatype requests.

A test is added which recreates what happened to trigger the MB.
A getMeta was returning key_enoent instead of the datatype because
it was doing a full bgFetch against a deleted value.

Change-Id: I6715d789f6cb8503cd44b860fd78ae3224d9bc67
Reviewed-on: http://review.couchbase.org/77182
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>

diff --git a/src/ep_engine.cc b/src/ep_engine.cc
index be4c038..1499f43 100644 (file)
--- a/src/ep_engine.cc
+++ b/src/ep_engine.cc
@@ -4909,14 +4909,14 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::getMeta(const void* cookie,
     uint32_t deleted = 0;
     uint8_t datatype = PROTOCOL_BINARY_RAW_BYTES;
     auto rv = kvBucket->getMetaData(
-            key, vbucket, cookie, fetchDatatype, metadata, deleted, datatype);
+            key, vbucket, cookie, metadata, deleted, datatype);
 
     if (rv == ENGINE_SUCCESS) {
         // GET META returns a packed structure
         // 0-3   uint32_t deleted
         // 4-7   uint32_t flags
         // 8-11  uint32_t expiry
-        // 12-19 uint32_t deleted
+        // 12-19 uint64_t seqno
         // 20    uint8_t  datatype (optional, based upon meta-version requested)
 
         uint8_t meta[(3 * sizeof(uint32_t)) + sizeof(uint64_t) +

diff --git a/src/kv_bucket.cc b/src/kv_bucket.cc
index 9d6608e..a498147 100644 (file)
--- a/src/kv_bucket.cc
+++ b/src/kv_bucket.cc
@@ -1498,7 +1498,6 @@ GetValue KVBucket::getRandomKey() {
 ENGINE_ERROR_CODE KVBucket::getMetaData(const DocKey& key,
                                         uint16_t vbucket,
                                         const void* cookie,
-                                        bool fetchDatatype,
                                         ItemMetaData& metadata,
                                         uint32_t& deleted,
                                         uint8_t& datatype)
@@ -1518,7 +1517,7 @@ ENGINE_ERROR_CODE KVBucket::getMetaData(const DocKey& key,
     }
 
     return vb->getMetaData(
-            key, cookie, engine, bgFetchDelay, fetchDatatype, metadata,
+            key, cookie, engine, bgFetchDelay, metadata,
             deleted, datatype);
 }
 

diff --git a/src/kv_bucket.h b/src/kv_bucket.h
index fd75837..e447620 100644 (file)
--- a/src/kv_bucket.h
+++ b/src/kv_bucket.h
@@ -183,7 +183,6 @@ public:
      * @parapm key the key to get the meta data for
      * @param vbucket the vbucket from which to retrieve the key
      * @param cookie the connection cookie
-     * @param fetchDatatype whether to fetch datatype or not
      * @param[out] metadata where to store the meta informaion
      * @param[out] deleted specifies whether or not the key is deleted
      * @param[out] datatype specifies the datatype for the given item
@@ -191,7 +190,6 @@ public:
     ENGINE_ERROR_CODE getMetaData(const DocKey& key,
                                   uint16_t vbucket,
                                   const void* cookie,
-                                  bool fetchDatatype,
                                   ItemMetaData& metadata,
                                   uint32_t& deleted,
                                   uint8_t& datatype);

diff --git a/src/kv_bucket_iface.h b/src/kv_bucket_iface.h
index 21e24c0..b79fafd 100644 (file)
--- a/src/kv_bucket_iface.h
+++ b/src/kv_bucket_iface.h
@@ -214,7 +214,6 @@ public:
      * @param key the key to get the meta data for
      * @param vbucket the vbucket from which to retrieve the key
      * @param cookie the connection cookie
-     * @param fetchDatatype whether to fetch datatype or not
      * @param[out] metadata where to store the meta informaion
      * @param[out] deleted specifies whether or not the key is deleted
      * @param[out] datatype specifies the datatype of the item
@@ -222,7 +221,6 @@ public:
     virtual ENGINE_ERROR_CODE getMetaData(const DocKey& key,
                                           uint16_t vbucket,
                                           const void* cookie,
-                                          bool fetchDatatype,
                                           ItemMetaData& metadata,
                                           uint32_t& deleted,
                                           uint8_t& datatype) = 0;

diff --git a/src/vbucket.cc b/src/vbucket.cc
index cef8e07..85c9094 100644 (file)
--- a/src/vbucket.cc
+++ b/src/vbucket.cc
@@ -1595,7 +1595,6 @@ ENGINE_ERROR_CODE VBucket::getMetaData(const DocKey& key,
                                        const void* cookie,
                                        EventuallyPersistentEngine& engine,
                                        int bgFetchDelay,
-                                       bool fetchDatatype,
                                        ItemMetaData& metadata,
                                        uint32_t& deleted,
                                        uint8_t& datatype) {
@@ -1606,9 +1605,9 @@ ENGINE_ERROR_CODE VBucket::getMetaData(const DocKey& key,
 
     if (v) {
         stats.numOpsGetMeta++;
-        if (v->isTempInitialItem() || (fetchDatatype && !v->isResident())) {
+        if (v->isTempInitialItem()) {
             // Need bg meta fetch.
-            bgFetch(key, cookie, engine, bgFetchDelay, !fetchDatatype);
+            bgFetch(key, cookie, engine, bgFetchDelay, true);
             return ENGINE_EWOULDBLOCK;
         } else if (v->isTempNonExistentItem()) {
             metadata.cas = v->getCas();
@@ -1627,13 +1626,7 @@ ENGINE_ERROR_CODE VBucket::getMetaData(const DocKey& key,
             metadata.flags = v->getFlags();
             metadata.exptime = v->getExptime();
             metadata.revSeqno = v->getRevSeqno();
-
-            if (fetchDatatype) {
-                value_t value = v->getValue();
-                if (value) {
-                    datatype = value->getDataType();
-                }
-            }
+            datatype = v->getDatatype();
 
             return ENGINE_SUCCESS;
         }
@@ -1649,7 +1642,7 @@ ENGINE_ERROR_CODE VBucket::getMetaData(const DocKey& key,
 
         if (maybeKeyExistsInFilter(key)) {
             return addTempItemAndBGFetch(
-                    hbl, key, cookie, engine, bgFetchDelay, !fetchDatatype);
+                    hbl, key, cookie, engine, bgFetchDelay, true);
         } else {
             stats.numOpsGetMeta++;
             return ENGINE_KEY_ENOENT;

diff --git a/src/vbucket.h b/src/vbucket.h
index 074c27b..6dbf767 100644 (file)
--- a/src/vbucket.h
+++ b/src/vbucket.h
@@ -1009,7 +1009,6 @@ public:
      * @param cookie the connection cookie
      * @param engine Reference to ep engine
      * @param bgFetchDelay Delay in secs before we run the bgFetch task
-     * @param fetchDatatype whether to fetch datatype or not
      * @param[out] metadata meta information returned to the caller
      * @param[out] deleted specifies the caller whether or not the key is
      *                     deleted
@@ -1021,7 +1020,6 @@ public:
                                   const void* cookie,
                                   EventuallyPersistentEngine& engine,
                                   int bgFetchDelay,
-                                  bool fetchDatatype,
                                   ItemMetaData& metadata,
                                   uint32_t& deleted,
                                   uint8_t& datatype);

diff --git a/tests/ep_testsuite_xdcr.cc b/tests/ep_testsuite_xdcr.cc
index 196aa51..6646efe 100644 (file)
--- a/tests/ep_testsuite_xdcr.cc
+++ b/tests/ep_testsuite_xdcr.cc
@@ -414,6 +414,53 @@ static enum test_result test_get_meta_with_xattr(ENGINE_HANDLE* h, ENGINE_HANDLE
     return SUCCESS;
 }
 
+/**
+ * Test that we can still get datatype of the deleted item after compaction
+ */
+static enum test_result test_get_meta_mb23905(ENGINE_HANDLE* h,
+                                            ENGINE_HANDLE_V1* h1)
+{
+    const char* key = "get_meta_key";
+    std::vector<char> data = createXattrValue({"test_expiry_value"});
+
+    const void* cookie = testHarness.create_cookie();
+
+    item *itm = nullptr;
+    checkeq(ENGINE_SUCCESS,
+            storeCasVb11(h, h1, cookie, OPERATION_SET, key,
+                         reinterpret_cast<char*>(data.data()),
+                         data.size(), 9258, &itm, 0, 0, 0,
+                         PROTOCOL_BINARY_DATATYPE_XATTR),
+            "Failed to store xattr document");
+    h1->release(h, nullptr, itm);
+
+    if (isPersistentBucket(h, h1)) {
+        wait_for_flusher_to_settle(h, h1);
+    }
+
+    if (isPersistentBucket(h, h1)) {
+        checkeq(ENGINE_SUCCESS, del(h, h1, key, 0, 0), "Delete failed");
+
+        // Run compaction to start using the bloomfilter
+        useconds_t sleepTime = 128;
+        compact_db(h, h1, 0, 0, 1, 1, 0);
+        while (get_int_stat(h, h1, "ep_pending_compactions") != 0) {
+            decayingSleep(&sleepTime);
+        }
+
+        checkeq(true,
+                get_meta(h, h1, key, true, GetMetaVersion::V2, cookie),
+                "Get meta command failed");
+
+        checkeq(static_cast<uint8_t>(PROTOCOL_BINARY_DATATYPE_XATTR),
+                last_datatype.load(), "Datatype is not XATTR");
+    }
+
+    testHarness.destroy_cookie(cookie);
+
+    return SUCCESS;
+}
+
 static enum test_result test_add_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1)
 {
     const char *key = "mykey";
@@ -2580,6 +2627,7 @@ 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, prepare_ep_bucket, cleanup),
         TestCase(NULL, NULL, NULL, NULL, NULL, prepare, cleanup)
 };

diff --git a/tests/module_tests/evp_store_single_threaded_test.cc b/tests/module_tests/evp_store_single_threaded_test.cc
index ad5c313..a92c872 100644 (file)
--- a/tests/module_tests/evp_store_single_threaded_test.cc
+++ b/tests/module_tests/evp_store_single_threaded_test.cc
@@ -1070,7 +1070,7 @@ TEST_F(SingleThreadedEPBucketTest, pre_expiry_xattrs) {
     ItemMetaData metadata;
     uint32_t deleted;
     uint8_t datatype;
-    kvbucket.getMetaData(makeStoredDocKey("key"), vbid, cookie, false, metadata,
+    kvbucket.getMetaData(makeStoredDocKey("key"), vbid, cookie, metadata,
                          deleted, datatype);
     auto prev_revseqno = metadata.revSeqno;
     EXPECT_EQ(1, prev_revseqno) << "Unexpected revision sequence number";
@@ -1099,7 +1099,7 @@ TEST_F(SingleThreadedEPBucketTest, pre_expiry_xattrs) {
 
     EXPECT_EQ(cas_str, sync_str) << "Unexpected system xattrs";
 
-    kvbucket.getMetaData(makeStoredDocKey("key"), vbid, cookie, false ,metadata,
+    kvbucket.getMetaData(makeStoredDocKey("key"), vbid, cookie, metadata,
                          deleted, datatype);
     EXPECT_EQ(prev_revseqno + 1, metadata.revSeqno) <<
              "Unexpected revision sequence number";

diff --git a/tests/module_tests/kv_bucket_test.cc b/tests/module_tests/kv_bucket_test.cc
index 6ede796..a585eaf 100644 (file)
--- a/tests/module_tests/kv_bucket_test.cc
+++ b/tests/module_tests/kv_bucket_test.cc
@@ -571,7 +571,7 @@ TEST_P(KVBucketParamTest, mb22824) {
     uint8_t datatype = PROTOCOL_BINARY_RAW_BYTES;
     EXPECT_EQ(ENGINE_SUCCESS,
               store->getMetaData(
-                      key, vbid, cookie, false, itemMeta1, deleted, datatype));
+                      key, vbid, cookie, itemMeta1, deleted, datatype));
 
     uint64_t cas = 0;
     ItemMetaData itemMeta2;