WT-6118 Missing checkpoint in backup (#5643)

* WT-6118 Missing checkpoint in backup

Change how we track the time of the most recent checkpoint.  Instead
of saving it after a checkpoint completes, use the time value we assign
when initializing a new checkpoint structure.  Rename the variable
where we store this to better reflect new usage.

Re-enable deletion of checkpoints during backup and the tests of that
functionality.

Co-authored-by: Keith A. Smith <keith.smith@mongodb>

src/block/block_ckpt.c

@@ -848,7 +848,7 @@ __ckpt_update(
      * an intermediate checkpoint rewritten because a checkpoint was rolled into it), even though
      * it's not necessary: those blocks aren't the last checkpoint in the file and so aren't
      * included in a recoverable checkpoint, they don't matter on a hot backup target until they're
-     * allocated and * used in the context of a live system. Regardless, they shouldn't materially
+     * allocated and used in the context of a live system. Regardless, they shouldn't materially
      * affect how much data we're writing, and it keeps things more consistent on the target to
      * update them. (Ignore the live system's ckpt_avail list here. The blocks on that list were
      * written into the final avail extent list which will be copied to the hot backup, and that's

src/conn/conn_open.c

@@ -39,7 +39,7 @@ __wt_connection_open(WT_CONNECTION_IMPL *conn, const char *cfg[])
      */
     conn->default_session = session;
 
-    __wt_seconds(session, &conn->ckpt_finish_secs);
+    __wt_seconds(session, &conn->ckpt_most_recent);
 
     /*
      * Publish: there must be a barrier to ensure the connection structure fields are set before

src/include/connection.h

@@ -152,7 +152,7 @@ struct __wt_named_extractor {
  */
 #define WT_CONN_HOTBACKUP_START(conn)                        \
     do {                                                     \
-        (conn)->hot_backup_start = (conn)->ckpt_finish_secs; \
+        (conn)->hot_backup_start = (conn)->ckpt_most_recent; \
         (conn)->hot_backup_list = NULL;                      \
     } while (0)
 
@@ -276,7 +276,7 @@ struct __wt_connection_impl {
     wt_thread_t ckpt_tid;          /* Checkpoint thread */
     bool ckpt_tid_set;             /* Checkpoint thread set */
     WT_CONDVAR *ckpt_cond;         /* Checkpoint wait mutex */
-    uint64_t ckpt_finish_secs;     /* Clock value of last completed checkpoint */
+    uint64_t ckpt_most_recent;     /* Clock value of most recent checkpoint */
 #define WT_CKPT_LOGSIZE(conn) ((conn)->ckpt_logsize != 0)
     wt_off_t ckpt_logsize; /* Checkpoint log size period */
     bool ckpt_signalled;   /* Checkpoint signalled */

src/meta/meta_ckpt.c

@@ -461,15 +461,18 @@ __wt_meta_ckptlist_get(
     WT_CKPT *ckpt, *ckptbase;
     WT_CONFIG ckptconf;
     WT_CONFIG_ITEM k, v;
+    WT_CONNECTION_IMPL *conn;
     WT_DECL_RET;
     size_t allocated, slot;
+    uint64_t most_recent;
     char *config;
 
     *ckptbasep = NULL;
 
     ckptbase = NULL;
     allocated = slot = 0;
     config = NULL;
+    conn = S2C(session);
 
     /* Retrieve the metadata information for the file. */
     WT_RET(__wt_metadata_search(session, fname, &config));
@@ -510,6 +513,17 @@ __wt_meta_ckptlist_get(
         ckpt = &ckptbase[slot];
         ckpt->order = (slot == 0) ? 1 : ckptbase[slot - 1].order + 1;
         __wt_seconds(session, &ckpt->sec);
+        /*
+         * Update time value for most recent checkpoint, not letting it move backwards. It is
+         * possible to race here, so use atomic CAS. This code relies on the fact that anyone we
+         * race with will only increase (never decrease) the most recent checkpoint time value.
+         */
+        for (;;) {
+            WT_ORDERED_READ(most_recent, conn->ckpt_most_recent);
+            if (ckpt->sec <= most_recent ||
+              __wt_atomic_cas64(&conn->ckpt_most_recent, most_recent, ckpt->sec))
+                break;
+        }
         /*
          * Load most recent checkpoint backup blocks to this checkpoint.
          */
@@ -522,7 +536,7 @@ __wt_meta_ckptlist_get(
          * the checkpoint's modified blocks from the block manager.
          */
         F_SET(ckpt, WT_CKPT_ADD);
-        if (F_ISSET(S2C(session), WT_CONN_INCR_BACKUP)) {
+        if (F_ISSET(conn, WT_CONN_INCR_BACKUP)) {
             F_SET(ckpt, WT_CKPT_BLOCK_MODS);
             WT_ERR(__ckpt_valid_blk_mods(session, ckpt));
         }

src/txn/txn_ckpt.c

@@ -745,8 +745,8 @@ __txn_checkpoint(WT_SESSION_IMPL *session, const char *cfg[])
     WT_TXN_GLOBAL *txn_global;
     WT_TXN_ISOLATION saved_isolation;
     wt_timestamp_t ckpt_tmp_ts;
-    uint64_t finish_secs, hs_ckpt_duration_usecs, time_start_hs, time_stop_hs;
-    uint64_t fsync_duration_usecs, generation, time_start_fsync, time_stop_fsync;
+    uint64_t fsync_duration_usecs, generation, hs_ckpt_duration_usecs;
+    uint64_t time_start_fsync, time_stop_fsync, time_start_hs, time_stop_hs;
     u_int i;
     bool can_skip, failed, full, idle, logging, tracking, use_timestamp;
     void *saved_meta_next;
@@ -985,16 +985,6 @@ __txn_checkpoint(WT_SESSION_IMPL *session, const char *cfg[])
                 conn->txn_global.last_ckpt_timestamp = conn->txn_global.recovery_timestamp;
         } else
             conn->txn_global.last_ckpt_timestamp = WT_TS_NONE;
-
-        /*
-         * Save clock value marking end of checkpoint processing. If a hot backup starts before the
-         * next checkpoint, we will need to keep all checkpoints up to this clock value until the
-         * backup completes.
-         */
-        __wt_seconds(session, &finish_secs);
-        /* Be defensive: time is only monotonic per session */
-        if (finish_secs > conn->ckpt_finish_secs)
-            conn->ckpt_finish_secs = finish_secs;
     }
 
 err:
@@ -1259,27 +1249,20 @@ __checkpoint_lock_dirty_tree_int(WT_SESSION_IMPL *session, bool is_checkpoint, b
             continue;
         is_wt_ckpt = WT_PREFIX_MATCH(ckpt->name, WT_CHECKPOINT);
 
-/*
- * If there is a hot backup, don't delete any WiredTiger checkpoint that could possibly have been
- * created before the backup started. Fail if trying to delete any other named checkpoint.
- */
-#ifdef DISABLED_CODE
-        if (conn->hot_backup_start != 0 && ckpt->sec <= conn->hot_backup_start) {
-#else
         /*
-         * N.B. Despite the comment above, dropping checkpoints during backup can corrupt the
-         * backup. For now we retain all WiredTiger checkpoints.
+         * If there is a hot backup, don't delete any WiredTiger checkpoint that could possibly have
+         * been created before the backup started. Fail if trying to delete any other named
+         * checkpoint.
          */
-        if (conn->hot_backup_start != 0) {
-#endif
+        if (conn->hot_backup_start != 0 && ckpt->sec <= conn->hot_backup_start) {
             if (is_wt_ckpt) {
                 F_CLR(ckpt, WT_CKPT_DELETE);
                 continue;
             }
             WT_RET_MSG(session, EBUSY,
               "checkpoint %s blocked by hot backup: it would "
-              "delete an existing checkpoint, and checkpoints "
-              "cannot be deleted during a hot backup",
+              "delete an existing named checkpoint, and such "
+              "checkpoints cannot be deleted during a hot backup",
               ckpt->name);
         }
         /*

test/suite/test_backup01.py

@@ -189,18 +189,16 @@ def test_checkpoint_delete(self):
             lambda: self.session.checkpoint("name=three,drop=(two)"), msg)
         self.session.checkpoint()
 
-        # Confirm that a named checkpoint created after a backup cursor can be dropped.
         # Need to pause a couple seconds; checkpoints that are assigned the same timestamp as
         # the backup will be pinned, even if they occur after the backup starts.
-
         time.sleep(2)
-        # N.B. This test is temporarily disabled because deleting checkpoints during backup
-        # can corrupt the backup.
-        #self.session.checkpoint("name=four")
-        #self.session.checkpoint("drop=(four)")
-        #self.assertRaises(wiredtiger.WiredTigerError,
-        #    lambda: self.session.open_cursor(
-        #    self.objs[0][0], None, "checkpoint=four"))
+
+        # Confirm that a named checkpoint created after a backup cursor can be dropped.
+        self.session.checkpoint("name=four")
+        self.session.checkpoint("drop=(four)")
+        self.assertRaises(wiredtiger.WiredTigerError,
+            lambda: self.session.open_cursor(
+            self.objs[0][0], None, "checkpoint=four"))
 
         # Confirm that after closing the backup cursor the original named checkpoint can
         # be deleted.

test/suite/test_checkpoint05.py

@@ -76,10 +76,7 @@ def test_checkpoints_during_backup(self):
         # is generous.  But if WT isn't deleting checkpoints there would
         # be about 30x more checkpoints here.
         final_count = self.count_checkpoints()
-
-        # N.B. This test is temporarily disabled because deleting checkpoints during backup
-        # can corrupt the backup.
-        # self.assertTrue (final_count < initial_count * 3)
+        self.assertTrue (final_count < initial_count * 3)
 
         self.session.close()