WT-5778 Simplify __rec_append_orig_value (#5458)

wiredtiger · Apr 2, 2020 · efae812 · efae812
1 parent 27f7862
commit efae812
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 33 deletions.
diff --git a/src/reconcile/rec_visibility.c b/src/reconcile/rec_visibility.c
@@ -61,6 +61,8 @@ __rec_append_orig_value(
     WT_UPDATE *append, *tombstone;
     size_t size, total_size;
 
+    WT_ASSERT(session, upd != NULL && unpack != NULL && unpack->type != WT_CELL_DEL);
+
     for (;; upd = upd->next) {
         /* Done if at least one self-contained update is globally visible. */
         if (WT_UPDATE_DATA_VALUE(upd) && __wt_txn_upd_visible_all(session, upd))
@@ -93,36 +95,38 @@ __rec_append_orig_value(
      */
     append = tombstone = NULL; /* -Wconditional-uninitialized */
     total_size = size = 0;     /* -Wconditional-uninitialized */
-    if (unpack == NULL || unpack->type == WT_CELL_DEL)
-        WT_RET(__wt_update_alloc(session, NULL, &append, &size, WT_UPDATE_TOMBSTONE));
-    else {
-        WT_RET(__wt_scr_alloc(session, 0, &tmp));
-        WT_ERR(__wt_page_cell_data_ref(session, page, unpack, tmp));
-        WT_ERR(__wt_update_alloc(session, tmp, &append, &size, WT_UPDATE_STANDARD));
-        append->start_ts = append->durable_ts = unpack->start_ts;
-        append->txnid = unpack->start_txn;
-        total_size = size;
 
-        /*
-         * We need to append a TOMBSTONE before the onpage value if the onpage value has a valid
-         * stop pair.
-         *
-         * Imagine a case we insert and delete a value respectively at timestamp 0 and 10, and later
-         * insert it again at 20. We need the TOMBSTONE to tell us there is no value between 10 and
-         * 20.
-         */
-        if (unpack->stop_ts != WT_TS_MAX || unpack->stop_txn != WT_TXN_MAX) {
-            WT_ERR(__wt_update_alloc(session, NULL, &tombstone, &size, WT_UPDATE_TOMBSTONE));
-            tombstone->txnid = unpack->stop_txn;
-            tombstone->start_ts = unpack->stop_ts;
-            tombstone->durable_ts = unpack->stop_ts;
-            tombstone->next = append;
-            total_size += size;
-        }
+    /*
+     * We need to append a TOMBSTONE before the onpage value if the onpage value has a valid
+     * stop pair.
+     *
+     * Imagine a case we insert and delete a value respectively at timestamp 0 and 10, and later
+     * insert it again at 20. We need the TOMBSTONE to tell us there is no value between 10 and
+     * 20.
+     */
+    if (unpack->stop_ts != WT_TS_MAX || unpack->stop_txn != WT_TXN_MAX) {
+        /* No need to append anything if the stop time pair is globally visible. */
+        if (__wt_txn_visible_all(session, unpack->stop_txn, unpack->stop_ts))
+            return (0);
+        WT_ERR(__wt_update_alloc(session, NULL, &tombstone, &size, WT_UPDATE_TOMBSTONE));
+        tombstone->txnid = unpack->stop_txn;
+        tombstone->start_ts = unpack->stop_ts;
+        tombstone->durable_ts = unpack->durable_stop_ts;
+        total_size += size;
     }
 
-    if (tombstone != NULL)
+    WT_ERR(__wt_scr_alloc(session, 0, &tmp));
+    WT_ERR(__wt_page_cell_data_ref(session, page, unpack, tmp));
+    WT_ERR(__wt_update_alloc(session, tmp, &append, &size, WT_UPDATE_STANDARD));
+    append->txnid = unpack->start_txn;
+    append->start_ts = unpack->start_ts;
+    append->durable_ts = unpack->durable_start_ts;
+    total_size += size;
+
+    if (tombstone != NULL) {
+        tombstone->next = append;
         append = tombstone;
+    }
 
     /* Append the new entry into the update list. */
     WT_PUBLISH(upd->next, append);
@@ -133,7 +137,8 @@ __rec_append_orig_value(
     __wt_scr_free(session, &tmp);
     /* Free append when tombstone allocation fails */
     if (ret != 0) {
-        __wt_free_update_list(session, &append);
+        __wt_free(session, append);
+        __wt_free(session, tombstone);
     }
     return (ret);
 }
@@ -401,7 +406,8 @@ __wt_rec_upd_select(WT_SESSION_IMPL *session, WT_RECONCILE *r, WT_INSERT *ins, v
             WT_ERR(__wt_scr_alloc(session, 0, &tmp));
             WT_ERR(__wt_page_cell_data_ref(session, page, vpack, tmp));
             WT_ERR(__wt_update_alloc(session, tmp, &upd, &size, WT_UPDATE_STANDARD));
-            upd->start_ts = upd->durable_ts = vpack->start_ts;
+            upd->durable_ts = vpack->durable_start_ts;
+            upd->start_ts = vpack->start_ts;
             upd->txnid = vpack->start_txn;
             WT_PUBLISH(last_upd->next, upd);
             /* This is going in our update list so it should be accounted for in cache usage. */
@@ -494,9 +500,7 @@ __wt_rec_upd_select(WT_SESSION_IMPL *session, WT_RECONCILE *r, WT_INSERT *ins, v
      * time there are saved updates and during reconciliation of a backing overflow record that will
      * be physically removed once it's no longer needed.
      */
-    if (upd_select->upd != NULL &&
-      (upd_saved || (vpack != NULL && F_ISSET(vpack, WT_CELL_UNPACK_OVERFLOW) &&
-                      vpack->raw != WT_CELL_VALUE_OVFL_RM)))
+    if (vpack != NULL && vpack->type != WT_CELL_DEL && upd_select->upd != NULL && upd_saved)
         WT_ERR(__rec_append_orig_value(session, page, upd_select->upd, vpack));
 
 err:

diff --git a/test/suite/test_rollback_to_stable06.py b/test/suite/test_rollback_to_stable06.py
@@ -118,12 +118,11 @@ def test_rollback_to_stable(self):
         self.assertEqual(calls, 1)
         self.assertEqual(keys_restored, 0)
         self.assertGreater(pages_visited, 0)
+        self.assertGreaterEqual(keys_removed, 0)
         if self.in_memory or self.prepare:
-            self.assertEqual(keys_removed, 0)
             self.assertEqual(upd_aborted, nrows * 4)
             self.assertEqual(hs_removed, 0)
         else:
-            self.assertGreaterEqual(keys_removed, 0)
             self.assertGreaterEqual(upd_aborted, 0)
             self.assertGreaterEqual(hs_removed, nrows * 3)