Fix off-by-one error with removeClosedUnrefCheckpoints.limit

There is an off-by-one error in
CheckpointManager::removeClosedUnrefCheckpoints() when the `limit`
argumeent is used - the last unreferenced Checkpoint is not removed.

This is because the early `break` from the for-loop doesn't increment
`it` - as it would normally be in the for loop iteration expression -
note that `it` indicates the first checkpoint to keep, *not* the last
one to remove (as per std::list::splice taking the range [first, last)
(i.e. last is exclusive).

For example, consider 2 checkpoints where the only cursors are in the
final checkpoint:

    1:CLOSED, 2:OPEN
              ^
              |
              Persistence cursor

We should be able to remove Checkpoint 1 when limit==1 is specified,
however due to the aforementioned bug we don't remove any.

Change-Id: I355b5f953bb8e22993e86fff2a11f2794fa368e6
Reviewed-on: http://review.couchbase.org/111654
Reviewed-by: James Harrison <[email protected]>
Tested-by: Build Bot <[email protected]>

Author: daverigby

engines/ep/benchmarks/vbucket_bench.cc

@@ -314,7 +314,8 @@ BENCHMARK_DEFINE_F(CheckpointBench, QueueDirtyWithManyClosedUnrefCheckpoints)
                             /*preLinkDocCtx*/ nullptr);
     }
 
-    // Simulate the Flusher, this makes all checkpoints eligible for removing
+    // Simulate the Flusher, this makes all but the current checkpoint eligible
+    // for removing (cannot remove checkpoint flusher is in).
     std::vector<queued_item> items;
     ckptMgr->getAllItemsForPersistence(items);
     ckptMgr->itemsPersisted();
@@ -342,7 +343,9 @@ BENCHMARK_DEFINE_F(CheckpointBench, QueueDirtyWithManyClosedUnrefCheckpoints)
             numUnrefItems += removed;
             numCkptRemoverRuns++;
 
-            if (numUnrefItems >= numCheckpoints) {
+            // Once all but the last item (in last checkpoint) is removed,
+            // break.
+            if (numUnrefItems >= numCheckpoints - 1) {
                 break;
             }
         }
@@ -404,6 +407,7 @@ static void FlushArguments(benchmark::internal::Benchmark* b) {
 BENCHMARK_REGISTER_F(MemTrackingVBucketBench, FlushVBucket)
         ->Apply(FlushArguments);
 
+// Arguments: numCheckpoints, numCkptToRemovePerIteration
 BENCHMARK_REGISTER_F(CheckpointBench, QueueDirtyWithManyClosedUnrefCheckpoints)
         ->Args({1000000, 1000})
         ->Iterations(1);

engines/ep/src/checkpoint.cc

@@ -374,7 +374,8 @@ int64_t Checkpoint::getMutationId(const CheckpointCursor& cursor) const {
 
 std::ostream& operator <<(std::ostream& os, const Checkpoint& c) {
     os << "Checkpoint[" << &c << "] with"
-       << " seqno:{" << c.getLowSeqno() << "," << c.getHighSeqno() << "}"
+       << " id:" << c.checkpointId << " seqno:{" << c.getLowSeqno() << ","
+       << c.getHighSeqno() << "}"
        << " snap:{" << c.getSnapshotStartSeqno() << ","
        << c.getSnapshotEndSeqno() << "}"
        << " state:" << to_string(c.getState()) << " items:[" << std::endl;

engines/ep/src/checkpoint_manager.cc

@@ -401,6 +401,8 @@ size_t CheckpointManager::removeClosedUnrefCheckpoints(
         size_t numCheckpointsRemoved = 0;
         // Iterate through the current checkpoints (from oldest to newest),
         // checking if the checkpoint can be removed.
+        // `it` is set to the first checkpoint we want to keep - all earlier
+        // ones are removed.
         auto it = checkpointList.begin();
         // Note terminating condition - we stop at one before the last
         // checkpoint - we must leave at least one checkpoint in existence.
@@ -420,6 +422,7 @@ size_t CheckpointManager::removeClosedUnrefCheckpoints(
                 ++numCheckpointsRemoved;
 
                 if (numCheckpointsRemoved >= limit) {
+                    ++it;
                     break;
                 }