MB-33919: Generate delete time for newly deleted items

Rather than allowing the existing expiry time to become the delete-time,
which is flawed as per the MB, allow the  VBQueueItemCtx to determine
how the delete-time should be set.

DCP consumer and the with-meta interface should not generate a
delete-time, the incoming 'replicated' value should be accepted. However
a value of zero is not allowed, and will be ignored/regenerated
regardless of the VBQueueItemCtx setting.

Genuine deletes/expirys should generate a new delete-time in-line with
the issues highlighted in the MB.

Change-Id: I7ea6ed8575eaa510a02cceb37a86628a28405fb5
Reviewed-on: http://review.couchbase.org/108393
Well-Formed: Build Bot <[email protected]>
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: jimwwalker

engines/ep/src/ep_engine.cc

@@ -1085,8 +1085,14 @@ static ENGINE_ERROR_CODE compactDB(EventuallyPersistentEngine* e,
         break;
     case ENGINE_EWOULDBLOCK:
         LOG(EXTENSION_LOG_NOTICE,
-            "Compaction of db file id: %d scheduled "
-                "(awaiting completion).", compactreq.db_file_id);
+            "Compaction of db file id:%d, purge_before_ts:%" PRIu64
+            ", purge_before_seq:%" PRIu64
+            ", drop_deletes:%d scheduled "
+            "(awaiting completion).",
+            compactreq.db_file_id,
+            compactreq.purge_before_ts,
+            compactreq.purge_before_seq,
+            compactreq.drop_deletes);
         e->storeEngineSpecific(cookie, req);
         return ENGINE_EWOULDBLOCK;
     case ENGINE_TMPFAIL:

engines/ep/src/ep_types.h

@@ -38,6 +38,7 @@ using queued_item = SingleThreadedRCPtr<Item, Item*, std::default_delete<Item>>;
 enum class GenerateBySeqno { No, Yes };
 enum class GenerateRevSeqno { No, Yes };
 enum class GenerateCas { No, Yes };
+enum class GenerateDeleteTime { No, Yes };
 enum class TrackCasDrift { No, Yes };
 enum class WantsDeleted { No, Yes };
 enum class TrackReference { No, Yes };

engines/ep/src/ephemeral_vb.cc

@@ -92,6 +92,7 @@ bool EphemeralVBucket::pageOut(const HashTable::HashBucketLock& lh,
     }
     VBQueueItemCtx queueCtx(GenerateBySeqno::Yes,
                             GenerateCas::Yes,
+                            GenerateDeleteTime::Yes,
                             TrackCasDrift::No,
                             /*isBackfill*/ false,
                             nullptr);

engines/ep/src/vbucket.cc

@@ -709,6 +709,7 @@ VBNotifyCtx VBucket::queueDirty(
         StoredValue& v,
         const GenerateBySeqno generateBySeqno,
         const GenerateCas generateCas,
+        const GenerateDeleteTime generateDeleteTime,
         const bool isBackfillItem,
         PreLinkDocumentContext* preLinkDocumentContext) {
     VBNotifyCtx notifyCtx;
@@ -720,7 +721,8 @@ VBNotifyCtx VBucket::queueDirty(
     // our tombstone purger can use to determine which tombstones to purge. A
     // DCP replicated or deleteWithMeta created delete may already have a time
     // assigned to it.
-    if (qi->isDeleted() && qi->getDeleteTime() == 0) {
+    if (qi->isDeleted() && (generateDeleteTime == GenerateDeleteTime::Yes ||
+                            qi->getExptime() == 0)) {
         qi->setExpTime(ep_real_time());
     }
 
@@ -1052,8 +1054,12 @@ ENGINE_ERROR_CODE VBucket::addBackfillItem(Item& itm,
         v->unlock();
     }
 
+    // MB-33919: DCP backfill - we must respect the value set by the producer
+    // unless it is zero, which queueDirty will fix-up. Not mutations and
+    // deletions come through this path.
     VBQueueItemCtx queueItmCtx(genBySeqno,
                                GenerateCas::No,
+                               GenerateDeleteTime::No,
                                TrackCasDrift::No,
                                /*isBackfillItem*/ true,
                                nullptr /* No pre link should happen */);
@@ -1176,8 +1182,12 @@ ENGINE_ERROR_CODE VBucket::setWithMeta(
         v->unlock();
     }
 
+    // MB-33919: Do not generate the delete-time - delete's can come through
+    // this path and the delete time from the input should be used (unless it
+    // is 0, where it must be regenerated)
     VBQueueItemCtx queueItmCtx(genBySeqno,
                                genCas,
+                               GenerateDeleteTime::No,
                                TrackCasDrift::Yes,
                                /*isBackfillItem*/ false,
                                nullptr /* No pre link step needed */);
@@ -1314,6 +1324,7 @@ ENGINE_ERROR_CODE VBucket::deleteItem(
                                   metadata,
                                   VBQueueItemCtx(GenerateBySeqno::Yes,
                                                  GenerateCas::Yes,
+                                                 GenerateDeleteTime::Yes,
                                                  TrackCasDrift::No,
                                                  /*isBackfillItem*/ false,
                                                  nullptr /* no pre link */),
@@ -1474,8 +1485,12 @@ ENGINE_ERROR_CODE VBucket::deleteWithMeta(
         delrv = MutationStatus::NeedBgFetch;
         metaBgFetch = false;
     } else {
+        // MB-33919: The incoming meta.exptime should be used as the delete-time
+        // so request GenerateDeleteTime::No, if the incoming value is 0, a new
+        // delete-time will be generated.
         VBQueueItemCtx queueItmCtx(genBySeqno,
                                    generateCas,
+                                   GenerateDeleteTime::No,
                                    TrackCasDrift::Yes,
                                    backfill,
                                    nullptr /* No pre link step needed */);
@@ -2482,8 +2497,10 @@ VBucket::processExpiredItem(const HashTable::HashBucketLock& hbl,
         return std::make_tuple(MutationStatus::NeedBgFetch,
                                &v,
                                queueDirty(v,
+
                                           GenerateBySeqno::Yes,
                                           GenerateCas::Yes,
+                                          GenerateDeleteTime::Yes,
                                           /*isBackfillItem*/ false));
     }
 
@@ -2506,6 +2523,7 @@ VBucket::processExpiredItem(const HashTable::HashBucketLock& hbl,
                                   onlyMarkDeleted,
                                   VBQueueItemCtx(GenerateBySeqno::Yes,
                                                  GenerateCas::Yes,
+                                                 GenerateDeleteTime::Yes,
                                                  TrackCasDrift::No,
                                                  /*isBackfillItem*/ false,
                                                  nullptr /* no pre link */),
@@ -2601,6 +2619,7 @@ VBNotifyCtx VBucket::queueDirty(StoredValue& v,
     return queueDirty(v,
                       queueItmCtx.genBySeqno,
                       queueItmCtx.genCas,
+                      queueItmCtx.generateDeleteTime,
                       queueItmCtx.isBackfillItem,
                       queueItmCtx.preLinkDocumentContext);
 }

engines/ep/src/vbucket.h

@@ -66,23 +66,59 @@ struct VBNotifyCtx {
 /**
  * Structure that holds info needed to queue an item in chkpt or vb backfill
  * queue
+ *
+ * GenerateDeleteTime - Only the queueing of items which are isDeleted() == true
+ * will this parameter have any affect. E.g. an add of an Item with this set to
+ * Yes will have no effect, whilst an explicit delete this parameter will have
+ * have an effect.
+ *
+ * Note that when queueing a delete with and expiry time of 0, the delete time
+ * is always generated. It is invalid to queue an Item with a 0 delete time,
+ * in this case the GenerateDeleteTime setting is ignored.
+ *
  */
 struct VBQueueItemCtx {
+    /**
+     * For code which needs to determine if a delete time should be generated
+     * that is for example del and set-with-meta, expiry, deletes this
+     * constructor allows for the GenerateDeleteTime to be set as Yes or No.
+     */
+    VBQueueItemCtx(GenerateBySeqno genBySeqno,
+                   GenerateCas genCas,
+                   GenerateDeleteTime generateDeleteTime,
+                   TrackCasDrift trackCasDrift,
+                   bool isBackfillItem,
+                   PreLinkDocumentContext* preLinkDocumentContext)
+        : genBySeqno(genBySeqno),
+          genCas(genCas),
+          generateDeleteTime(generateDeleteTime),
+          trackCasDrift(trackCasDrift),
+          isBackfillItem(isBackfillItem),
+          preLinkDocumentContext(preLinkDocumentContext) {
+    }
+
+    /**
+     * For code which is queueing Item's that are not deleted and the generate
+     * delete time has no affect, this constructor should be used.
+     */
     VBQueueItemCtx(GenerateBySeqno genBySeqno,
                    GenerateCas genCas,
                    TrackCasDrift trackCasDrift,
                    bool isBackfillItem,
-                   PreLinkDocumentContext* preLinkDocumentContext_)
+                   PreLinkDocumentContext* preLinkDocumentContext)
         : genBySeqno(genBySeqno),
           genCas(genCas),
+          generateDeleteTime(GenerateDeleteTime::No),
           trackCasDrift(trackCasDrift),
           isBackfillItem(isBackfillItem),
-          preLinkDocumentContext(preLinkDocumentContext_) {
+          preLinkDocumentContext(preLinkDocumentContext) {
     }
+
     /* Indicates if we should queue an item or not. If this is false other
        members should not be used */
     GenerateBySeqno genBySeqno;
     GenerateCas genCas;
+    GenerateDeleteTime generateDeleteTime;
     TrackCasDrift trackCasDrift;
     bool isBackfillItem;
     PreLinkDocumentContext* preLinkDocumentContext;
@@ -1441,6 +1477,8 @@ class VBucket : public std::enable_shared_from_this<VBucket> {
      *          flags passed
      * @param generateBySeqno request that the seqno is generated by this call
      * @param generateCas request that the CAS is generated by this call
+     * @param generateDeleteTime for queueing deletes, should the queue item
+     *        have its delete time generated.
      * @param isBackfillItem indicates if the item must be put onto vb queue or
      *        onto checkpoint
      * @param preLinkDocumentContext context object which allows running the
@@ -1454,6 +1492,7 @@ class VBucket : public std::enable_shared_from_this<VBucket> {
             StoredValue& v,
             GenerateBySeqno generateBySeqno = GenerateBySeqno::Yes,
             GenerateCas generateCas = GenerateCas::Yes,
+            GenerateDeleteTime generateDeleteTime = GenerateDeleteTime::Yes,
             bool isBackfillItem = false,
             PreLinkDocumentContext* preLinkDocumentContext = nullptr);
 

engines/ep/tests/ep_testsuite.cc

@@ -1487,6 +1487,69 @@ static enum test_result test_vbucket_compact(ENGINE_HANDLE *h,
     return SUCCESS;
 }
 
+static enum test_result test_MB_33919(ENGINE_HANDLE* h, ENGINE_HANDLE_V1* h1) {
+    const char* expKey = "expiryKey";
+    const char* otherKey = "otherKey";
+    const char* value = "value";
+
+    // Test runs in the future as we need to set times in the past without
+    // falling foul of the expiry time being behind server start
+    auto fiveDays =
+            std::chrono::system_clock::now() + std::chrono::hours(5 * 24);
+
+    // Calculate expiry and purgeBefore times
+    auto threeDaysAgo = std::chrono::system_clock::to_time_t(
+            fiveDays - std::chrono::hours(3 * 24));
+    auto fourDaysAgo = std::chrono::system_clock::to_time_t(
+            fiveDays - std::chrono::hours(4 * 24));
+    // Time travel forwards by 5 days to give the illusion of 5 days uptime.
+    testHarness.time_travel((86400 * 5));
+
+    // Set expiring key, expiry 4 days ago
+    checkeq(ENGINE_SUCCESS,
+            store(h,
+                  h1,
+                  nullptr,
+                  OPERATION_SET,
+                  expKey,
+                  value,
+                  nullptr,
+                  0,
+                  0,
+                  fourDaysAgo),
+            "Failed to set expiring key");
+
+    // Force it to expire
+    ENGINE_ERROR_CODE val = verify_key(h, h1, expKey);
+    checkeq(ENGINE_KEY_ENOENT, val, "expiryKey has not expired.");
+
+    // And a second key (after expiry), compaction won't purge the high-seqno
+    checkeq(ENGINE_SUCCESS,
+            store(h, h1, nullptr, OPERATION_SET, otherKey, value),
+            "Failed to set non-expiring key");
+
+    // Wait for the item to be expired
+    wait_for_stat_to_be(h, h1, "vb_active_expired", 1);
+
+    wait_for_flusher_to_settle(h, h1);
+
+    checkeq(0,
+            get_int_stat(h, h1, "vb_0:purge_seqno", "vbucket-seqno"),
+            "purge_seqno not found to be zero before compaction");
+
+    // Compaction on VBucket
+    compact_db(
+            h, h1, 0 /* vbucket_id */, 0 /* db_file_id */, threeDaysAgo, 0, 0);
+    wait_for_stat_to_be(h, h1, "ep_pending_compactions", 0);
+
+    // Purge-seqno should not of moved, without the fix it would
+    checkeq(0,
+            get_int_stat(h, h1, "vb_0:purge_seqno", "vbucket-seqno"),
+            "purge_seqno not found to be zero before compaction");
+
+    return SUCCESS;
+}
+
 static enum test_result test_compaction_config(ENGINE_HANDLE *h,
                                                ENGINE_HANDLE_V1 *h1) {
 
@@ -8169,6 +8232,12 @@ BaseTestCase testsuite_testcases[] = {
         TestCase("test_MB-23640_get_document_of_any_state", test_mb23640,
                  test_setup, teardown, nullptr, prepare, cleanup),
 
-        TestCase(NULL, NULL, NULL, NULL, NULL, prepare, cleanup)
-};
+        TestCase("test MB-33919 past tombstone",
+                 test_MB_33919,
+                 test_setup,
+                 teardown,
+                 nullptr,
+                 prepare,
+                 cleanup),
 
+        TestCase(NULL, NULL, NULL, NULL, NULL, prepare, cleanup)};

engines/ep/tests/module_tests/evp_store_single_threaded_test.cc

@@ -2755,8 +2755,9 @@ TEST_F(WarmupTest, produce_delete_times) {
                      (sizeof("KEY2") - 1);
     EXPECT_EQ(expectedBytes, producer->getBytesOutstanding());
 
-    // Finally expire a key and check that the delete_time we receive is the
-    // expiry time, not actually the time it was deleted.
+    // Finally expire a key and check that the delete_time we receive is not the
+    // expiry time, the delete time should always be re-created by the server to
+    // ensure old/future expiry times don't disrupt tombstone purging (MB-33919)
     auto expiryTime = ep_real_time() + 32000;
     store_item(vbid,
                {"KEY3", DocNamespace::DefaultCollection},
@@ -2779,7 +2780,7 @@ TEST_F(WarmupTest, produce_delete_times) {
 
     step(true);
 
-    EXPECT_EQ(expiryTime, dcp_last_delete_time);
+    EXPECT_NE(expiryTime, dcp_last_delete_time);
     EXPECT_EQ(PROTOCOL_BINARY_CMD_DCP_DELETION, dcp_last_op);
     EXPECT_EQ("KEY3", dcp_last_key);
     expectedBytes += SnapshotMarker::baseMsgBytes +

engines/ep/tests/module_tests/evp_store_with_meta.cc

@@ -1356,6 +1356,42 @@ TEST_P(DelWithMetaTest, setting_deleteTime) {
     EXPECT_EQ(itemMeta.exptime, metadata.exptime);
 }
 
+// Perform a DeleteWithMeta with a deleteTime of 0 and verify that a time is
+// generated.
+TEST_P(DelWithMetaTest, setting_zero_deleteTime) {
+    ItemMetaData itemMeta{0xdeadbeef, 0xf00dcafe, 0xfacefeed, 0};
+    oneOpAndCheck(op,
+                  itemMeta,
+                  0, // no-options
+                  withValue,
+                  PROTOCOL_BINARY_RESPONSE_SUCCESS,
+                  withValue ? ENGINE_SUCCESS : ENGINE_EWOULDBLOCK);
+
+    EXPECT_EQ(std::make_pair(false, size_t(1)),
+              getEPBucket().flushVBucket(vbid));
+
+    ItemMetaData metadata;
+    uint32_t deleted = 0;
+    uint8_t datatype = 0;
+    EXPECT_EQ(ENGINE_EWOULDBLOCK,
+              store->getMetaData({"mykey", DocNamespace::DefaultCollection},
+                                 vbid,
+                                 cookie,
+                                 metadata,
+                                 deleted,
+                                 datatype));
+    MockGlobalTask mockTask(engine->getTaskable(), TaskId::MultiBGFetcherTask);
+    store->getVBucket(vbid)->getShard()->getBgFetcher()->run(&mockTask);
+    EXPECT_EQ(ENGINE_SUCCESS,
+              store->getMetaData({"mykey", DocNamespace::DefaultCollection},
+                                 vbid,
+                                 cookie,
+                                 metadata,
+                                 deleted,
+                                 datatype));
+    EXPECT_NE(0, metadata.exptime);
+}
+
 TEST_P(DelWithMetaTest, MB_31141) {
     ItemMetaData itemMeta{0xdeadbeef, 0xf00dcafe, 0xfacefeed, expiry};
     // Do a delete with valid extended meta

engines/ep/tests/module_tests/vbucket_test.cc

@@ -234,6 +234,7 @@ MutationStatus VBucketTestBase::public_processSoftDelete(const DocKey& key,
             metadata,
             VBQueueItemCtx(GenerateBySeqno::Yes,
                            GenerateCas::Yes,
+                           GenerateDeleteTime::Yes,
                            TrackCasDrift::No,
                            /*isBackfillItem*/ false,
                            /*preLinkDocCtx*/ nullptr),