MB-21925: Fix queue_fill stat when persitenceCursor on queue_op::empty

There is a bug related to how we account for items when we enqueue
mutations to be persisted (Checkpoint::queueDirty). The bug occurs
when we enqueue an item which already exists (i.e. it is to be
de-duplicated), and the persistence cursor has not permitted any items
for that VBucket - i.e. at the very beginning of the lifetime of the
VBucket.

With the advent of the MB-20852 (the set_vbucket_state changes), the
first item in a Bucket's checkpoint is now a
queue_op::set_vbucket_state - previously it would have been a "normal"
item. When items are added to a checkpoint, we just update the queuing
stats - essentially there are two possibilities:

1. The item is a new key (NEW_KEY), or it's a key which isn't new but
   has already been persisted (PERSIST_AGAIN) - we need to persist it -
   increment queue_fill

2. The item can be de-duplcated (EXISTING_ITEM) - don't increment
   queue_fill.

Essentially, there's a bug in this logic - we incorrectly mark the
incoming item as PERSIST_AGAIN, instead of EXISTING_ITEM. As a result
the queue_fill value is incorrectly inflated.

This is due to using a unsigned value for the mutation_id, which we
decrement by one if it's a meta_item. This should go to -1 (less than
any possible valid item id), but instead it goes to 2^63-1, *greater*
than any possible valid item (!)

Note: this is purely a stats issue, the actual item /is/ queued and
persisted correctly.

Change-Id: I8d40c417dc165e192b4f90e8cc16c6e87557d451
Reviewed-on: http://review.couchbase.org/70730
Tested-by: buildbot <build@couchbase.com>
Well-Formed: buildbot <build@couchbase.com>
Reviewed-by: Daniel Owen <owend@couchbase.com>

diff --git a/src/checkpoint.cc b/src/checkpoint.cc
index 1486af7..2e5e3cb 100644 (file)
--- a/src/checkpoint.cc
+++ b/src/checkpoint.cc
@@ -166,7 +166,7 @@ queue_dirty_t Checkpoint::queueDirty(const queued_item &qi,
         if (it != keyIndex.end()) {
             rv = EXISTING_ITEM;
             std::list<queued_item>::iterator currPos = it->second.position;
-            uint64_t currMutationId = it->second.mutation_id;
+            const int64_t currMutationId{it->second.mutation_id};
 
             // Given the key already exists, need to check all cursors in this
             // Checkpoint and see if the existing item for this key is to
@@ -200,7 +200,7 @@ queue_dirty_t Checkpoint::queueDirty(const queued_item &qi,
                     // decrement if the the existing item is strictly less than
                     // the cursor, as meta-items can share a seqno with
                     // a non-meta item but are logically before them.
-                    uint64_t cursor_mutation_id = cursor_item_idx->second.mutation_id;
+                    int64_t cursor_mutation_id{cursor_item_idx->second.mutation_id};
                     if (cursor_item->isCheckPointMetaItem()) {
                         --cursor_mutation_id;
                     }

diff --git a/src/checkpoint.h b/src/checkpoint.h
index 0698dd3..2f6de8b 100644 (file)
--- a/src/checkpoint.h
+++ b/src/checkpoint.h
@@ -241,15 +241,18 @@ typedef std::map<const std::string, CheckpointCursor> cursor_index;
  */
 enum queue_dirty_t {
     /*
-     * The item exists on the right hand side of the persistence cursor. The
-     * item will be deduplicated and doesn't change the size of the checkpoint.
+     * The item exists on the right hand side of the persistence cursor - i.e.
+     * the persistence cursor has not yet processed this key.
+     * The item will be deduplicated and doesn't change the size of the checkpoint.
      */
     EXISTING_ITEM,
 
     /**
-     * The item exists on the left hand side of the persistence cursor. It will
-     * be dedeuplicated and moved the to right hand side, but the item needs
-     * to be re-persisted.
+     * The item exists on the left hand side of the persistence cursor - i.e.
+     * the persistence cursor has already processed one (or more) instances of
+     * this key.
+     * It will be dedeuplicated and moved the to right hand side, but the item
+     * needs to be re-persisted.
      */
     PERSIST_AGAIN,
 

diff --git a/tests/module_tests/checkpoint_test.cc b/tests/module_tests/checkpoint_test.cc
index e8f529e..b60e8c6 100644 (file)
--- a/tests/module_tests/checkpoint_test.cc
+++ b/tests/module_tests/checkpoint_test.cc
@@ -69,10 +69,10 @@ protected:
         createManager();
     }
 
-    void createManager() {
+    void createManager(int64_t last_seqno = 1000) {
         manager.reset(new CheckpointManager(global_stats, vbucket->getId(),
                                             checkpoint_config,
-                                            /*lastSeqno*/1000,
+                                            last_seqno,
                                             /*lastSnapStart*/0,/*lastSnapEnd*/0,
                                             callback));
     }
@@ -1046,3 +1046,22 @@ TEST_F(CheckpointTest, CursorUpdateForExistingItemWithNonMetaItemAtHead) {
     EXPECT_EQ(1002, items.at(0)->getBySeqno());
     EXPECT_EQ("key", items.at(0)->getKey());
 }
+
+// Regression test for MB-21925 - when a duplicate key is queued and the
+// persistence cursor is still positioned on the initial dummy key,
+// should return EXISTING_ITEM.
+TEST_F(CheckpointTest,
+       MB21925_QueueDuplicateWithPersistenceCursorOnInitialMetaItem) {
+
+    // Need a manager starting from seqno zero.
+    createManager(0);
+    ASSERT_EQ(0, manager->getHighSeqno());
+    ASSERT_EQ(1, manager->getNumItems())
+        << "Should start with queue_op::empty on checkpoint.";
+
+    // Add an item with some new key.
+    ASSERT_TRUE(queueNewItem("key"));
+
+    // Test - second item (duplicate key) should return false.
+    EXPECT_FALSE(queueNewItem("key"));
+}