Fix the case where there are empty nodes during recovery

Author: Ravi Nagarjun Akella

src/include/homestore/btree/detail/btree_node.hpp

@@ -310,7 +310,7 @@ class BtreeNode : public sisl::ObjLifeCounter< BtreeNode > {
             auto prevKey = get_nth_key< K >(i - 1, false);
             auto curKey = get_nth_key< K >(i, false);
             if (prevKey.compare(curKey) >= 0) {
-                DEBUG_ASSERT(false, "Order check failed at entry={}", i);
+                DEBUG_ASSERT(false, "Order check failed at entry={}, btree id={}", i, node_id());
                 return false;
             }
         }

src/include/homestore/index/index_table.hpp

@@ -618,19 +618,24 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
             K last_child_last_key;
             K last_child_neighbor_key;
             BtreeNodePtr cur_child = child_node;
+            auto sibling_first_child = true_sibling_first_child(parent_node);
+            LOGTRACEMOD(wbcache, "Sibling first child id is {}", sibling_first_child);
 
             // We find the last child node by starting from the leftmost child and traversing through the
             // next_bnode links until we reach the end or find a sibling first child.
             bool found_child = false;
-            auto sibling_first_child = true_sibling_first_child(parent_node);
-            LOGTRACEMOD(wbcache, "Sibling first child id is {}", sibling_first_child);
             while (cur_child != nullptr) {
                 LOGTRACEMOD(wbcache, "Processing child node [{}]", cur_child->to_string());
                 if (!cur_child->is_node_deleted() && cur_child->total_entries() > 0) {
                     last_child_last_key = cur_child->get_last_key< K >();
                     found_child = true;
                 }
 
+                if (child_node->total_entries() == 0 && orig_child_infos.contains(child_node->node_id())) {
+                    last_child_last_key = orig_child_infos[child_node->node_id()];
+                    found_child = true;
+                }
+
                 next_cur_child = nullptr;
                 if (cur_child->next_bnode() == empty_bnodeid ||
                     read_node_impl(cur_child->next_bnode(), next_cur_child) != btree_status_t::success) {
@@ -685,7 +690,6 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
                     LOGTRACEMOD(wbcache,
                                 "No undeleted child found for parent_node [{}], keep normal repair (regular recovery)",
                                 parent_node->to_string());
-                    next_cur_child = nullptr;
                 }
             }
         }
@@ -700,6 +704,7 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
         // Walk across all child nodes until it gets the last_parent_key and keep fixing them.
         auto cur_parent = parent_node;
         BtreeNodeList new_parent_nodes;
+        BtreeNodeList child_nodes_to_free;
         do {
             if (child_node->has_valid_edge() || (child_node->is_leaf() && child_node->next_bnode() == empty_bnodeid)) {
                 LOGTRACEMOD(wbcache, "Child node [{}] is an edge node or a leaf with no next", child_node->to_string());
@@ -760,61 +765,6 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
             LOGTRACEMOD(wbcache, "Repairing node={}, child_node=[{}] child_last_key={}", cur_parent->node_id(),
                         child_node->to_string(), child_last_key.to_string());
 
-            // Check if we are beyond the last child node.
-            //
-            // There can be cases where the child level merge is successfully persisted but the parent level is
-            // not. In this case, you may have your rightmost child node with last key greater than the
-            // last_parent_key. That's why here we have to check if the child node is one of the original child
-            // nodes first.
-            if (!is_parent_edge_node && !orig_child_infos.contains(child_node->node_id())) {
-                LOGTRACEMOD(
-                    wbcache,
-                    "Child node [{}] is not one of the original child nodes, so we need to check if it is beyond the "
-                    "last parent key {}",
-                    child_node->to_string(), last_parent_key.to_string());
-                if (child_last_key.compare(last_parent_key) > 0) {
-                    // We have reached a child beyond this parent, we can stop now
-                    // TODO this case if child last key is less than last parent key to update the parent node.
-                    // this case can potentially break the btree for put and remove op.
-                    break;
-                }
-                if (child_node->total_entries() == 0) {
-                    // this child has no entries, but maybe in the middle of the parent node, we need to update the key
-                    // of parent as previous one and go on
-                    LOGTRACEMOD(wbcache,
-                                "Reach to an empty child node {}, and this child doesn't belong to this parent; Hence "
-                                "loop ends",
-                                child_node->to_string());
-                    // now update  the next of parent node by skipping all deleted siblings of this parent node
-                    auto valid_sibling = cur_parent->next_bnode();
-                    while (valid_sibling != empty_bnodeid) {
-                        BtreeNodePtr sibling;
-                        if (read_node_impl(valid_sibling, sibling) == btree_status_t::success) {
-                            if (sibling->is_node_deleted()) {
-                                valid_sibling = sibling->next_bnode();
-                                continue;
-                            }
-                            // cur_parent->set_next_bnode(sibling->node_id());
-                            break;
-                        }
-                        LOGTRACEMOD(wbcache, "Failed to read child node {} for parent node [{}] reason {}",
-                                    valid_sibling, cur_parent->to_string(), ret);
-                    }
-                    if (valid_sibling != empty_bnodeid) {
-                        cur_parent->set_next_bnode(valid_sibling);
-                        LOGTRACEMOD(wbcache, "Repairing node=[{}], child_node=[{}] is an edge node, end loop",
-                                    cur_parent->to_string(), child_node->to_string());
-
-                    } else {
-                        cur_parent->set_next_bnode(empty_bnodeid);
-                        LOGTRACEMOD(wbcache, "Repairing node=[{}], child_node=[{}] is an edge node, end loop",
-                                    cur_parent->to_string(), child_node->to_string());
-                    }
-
-                    break;
-                }
-            }
-
             if (!cur_parent->has_room_for_put(btree_put_type::INSERT, K::get_max_size(),
                                               BtreeLinkInfo::get_fixed_size())) {
                 // No room in the parent_node, let us split the parent_node and continue
@@ -836,22 +786,25 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
                 cur_parent = std::move(new_parent);
             }
 
+            // If the child node is empty, we mark it as deleted and remove them after the repair loop
+            if (child_node->total_entries() == 0) {
+                LOGTRACEMOD(wbcache, "Found an empty child node {}, marking it deleted",
+                            child_node->to_string());
+                child_node->set_node_deleted();
+                child_nodes_to_free.push_back(child_node);
+                // the links to the previous child node will be fixed in the next code block
+            }
+            
+
             // Insert the last key of the child node into parent node
             if (!child_node->is_node_deleted()) {
                 if (child_node->total_entries() == 0) {
-                    if (orig_child_infos.contains(child_node->node_id())) {
-                        child_last_key = orig_child_infos[child_node->node_id()];
-                        LOGTRACEMOD(wbcache,
-                                    "Reach to an empty child node [{}], but not the end of the parent node, so we need "
-                                    "to update the key of parent node as original one {}",
-                                    child_node->to_string(), child_last_key.to_string());
-                    } else {
-                        LOGTRACEMOD(wbcache,
-                                    "Reach to an empty child node [{}] but not belonging to this parent (probably next "
-                                    "parent sibling); Hence end loop",
-                                    child_node->to_string());
-                        break;
-                    }
+                    BT_LOG_ASSERT(false,
+                                    "Child node [{}] is empty but not deleted and not an edge, while it doesn't "
+                                    "belong to this parent node {}",
+                                    child_node->to_string(), parent_node->to_string());
+                    ret = btree_status_t::not_found;
+                    break;
                 }
                 cur_parent->insert(cur_parent->total_entries(), child_last_key,
                                    BtreeLinkInfo{child_node->node_id(), child_node->link_version()});
@@ -873,13 +826,6 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
                     IndexBtreeNode* idx_node = static_cast< IndexBtreeNode* >(pre_child_node.get());
                     idx_node->m_idx_buf->set_state(index_buf_state_t::CLEAN);
                     write_node_impl(pre_child_node, cp_ctx);
-                    // update the key of last entry of the parent with the last key of deleted child
-                    child_last_key = orig_child_infos[child_node->node_id()];
-                    LOGTRACEMOD(wbcache, "updating parent [{}] current last key with {}", cur_parent->to_string(),
-                                child_last_key.to_string());
-                    // update it here to go to the next child node and unlock this node
-                    LOGTRACEMOD(wbcache, "update the child node next to the next of previous child node");
-                    child_node->set_next_bnode(child_node->next_bnode());
                 }
             }
 
@@ -914,6 +860,7 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
                 child_node = nullptr;
                 break;
             }
+
             ret = this->read_and_lock_node(next_node_id, child_node, locktype_t::READ, locktype_t::READ, cp_ctx);
             if (ret != btree_status_t::success) {
                 BT_LOG_ASSERT(false, "Parent node={} repair is partial, because child_node get has failed with ret={}",
@@ -925,6 +872,10 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
         } while (true);
 
         if (child_node) { this->unlock_node(child_node, locktype_t::READ); }
+
+        // free the deleted child nodes
+        for (const auto& cnode : child_nodes_to_free) { free_node_impl(cnode, cp_ctx); }
+
         // if last parent has the key less than the last child key, then we need to update the parent node with
         // the last child key if it doesn't have edge.
         auto last_parent = parent_node;
@@ -996,7 +947,7 @@ class IndexTable : public IndexTableBase, public Btree< K, V > {
             }
 
             if (sibling_node->is_node_deleted()) {
-                LOGTRACEMOD(wbcache, "Sibling node [{}] is not the sibling for parent_node {}",
+                LOGTRACEMOD(wbcache, "Sibling node [{}] is not the true sibling for parent_node {}",
                             sibling_node->to_string(), node->to_string());
                 return find_true_sibling(sibling_node);
             } else {

src/tests/btree_helpers/btree_test_helper.hpp

@@ -329,8 +329,8 @@ struct BtreeTestHelper {
         do_range_remove(start_k, end_k, false /* removing_all_existing */);
     }
 
-    void range_remove_all(uint32_t start_k, uint32_t end_k) {
-        do_range_remove(start_k, end_k, true /* removing_all_existing */);
+    void range_remove_all(uint32_t start_k, uint32_t end_k, bool expect_success = true) {
+        do_range_remove(start_k, end_k, true /* removing_all_existing */, expect_success);
     }
 
     ////////////////////// All query operation variants ///////////////////////////////
@@ -532,7 +532,7 @@ struct BtreeTestHelper {
         if (expect_success) { m_shadow_map.put_and_check(key, value, *existing_v, done); }
     }
 
-    void do_range_remove(uint64_t start_k, uint64_t end_k, bool all_existing) {
+    void do_range_remove(uint64_t start_k, uint64_t end_k, bool all_existing, bool expect_success = true) {
         K start_key = K{start_k};
         K end_key = K{end_k};
 
@@ -542,8 +542,10 @@ struct BtreeTestHelper {
 
         if (all_existing) {
             m_shadow_map.range_erase(start_key, end_key);
-            ASSERT_EQ((ret == btree_status_t::success), true)
-                << "not a successful remove op for range " << start_k << "-" << end_k;
+            if (expect_success) {
+                ASSERT_EQ(ret, btree_status_t::success)
+                    << "not a successful remove op for range " << start_k << "-" << end_k;
+            }
         } else if (start_k < m_max_range_input) {
             K end_range{std::min(end_k, uint64_cast(m_max_range_input - 1))};
             m_shadow_map.range_erase(start_key, end_range);

src/tests/btree_helpers/btree_test_kvs.hpp

@@ -81,7 +81,7 @@ class TestFixedKey : public BtreeKey {
 public:
     TestFixedKey() = default;
     TestFixedKey(uint64_t k) : m_key{k} {}
-    TestFixedKey(const TestFixedKey& other) : TestFixedKey(other.serialize(), true) {}
+    TestFixedKey(const TestFixedKey& other) : m_key{other.m_key} {}
     TestFixedKey(const BtreeKey& other) : TestFixedKey(other.serialize(), true) {}
     TestFixedKey(const sisl::blob& b, bool copy) : BtreeKey(), m_key{*(r_cast< const uint64_t* >(b.cbytes()))} {}
     TestFixedKey& operator=(const TestFixedKey& other) = default;

src/tests/btree_helpers/shadow_map.hpp

@@ -70,16 +70,17 @@ class ShadowMap {
         return std::pair(start_it->first, it->first);
     }
 
-    std::pair< K, K > pick_existing_range(const K& start_key, uint32_t max_count, pick_existing_range_cb_t cb) const {
+    std::optional<std::pair< K, K > > pick_existing_range(const K& start_key, uint32_t max_count, pick_existing_range_cb_t cb) const {
         std::lock_guard lock{m_mutex};
         auto const start_it = m_map.lower_bound(start_key);
+        if (start_it == m_map.cend()) { return std::nullopt; }
+        auto end_it = start_it;
         auto it = start_it;
-        uint32_t count = 0;
-        while ((it != m_map.cend()) && (++count < max_count)) {
+        for(uint32_t count = 0; count < max_count && it != m_map.cend(); ++count, ++it) {
             cb(it->first);
-            ++it;
+            end_it = it;
         }
-        return std::pair(start_it->first, it->first);
+        return std::pair(start_it->first, end_it->first);
     }
 
     uint32_t max_keys() const { return m_max_keys; }

src/tests/test_index_crash_recovery.cpp

@@ -437,7 +437,7 @@ struct IndexCrashTest : public test_common::HSTestHelper, BtreeTestHelper< TestT
         if (range_remove_keys) {
             auto [s_key, e_key] = *range_remove_keys;
             LOGDEBUG("Reapply: Range removing keys [{}, {})", s_key, e_key);
-            this->range_remove_all(s_key, e_key);
+            this->range_remove_all(s_key, e_key, false /* expect_success*/);
         }
         trigger_cp(true);
     }
@@ -555,11 +555,15 @@ struct IndexCrashTest : public test_common::HSTestHelper, BtreeTestHelper< TestT
 
     uint32_t tree_key_count() { return this->m_bt->count_keys(this->m_bt->root_node_id()); }
 
-    std::pair<uint32_t, uint32_t> range_remove_op(SequenceGenerator& generator, uint32_t range_remove_count) {
+    std::optional< std::pair< uint32_t, uint32_t > > range_remove_op(SequenceGenerator& generator, uint32_t range_remove_count) {
         uint32_t key = generator.getKeyDistribution()(g_re); 
-        auto [start_key, end_key] = this->m_shadow_map.pick_existing_range(K{key}, range_remove_count,
+        auto range_opt = this->m_shadow_map.pick_existing_range(K{key}, range_remove_count,
                 [&generator](const K& key) { generator.getKeyStates()[key.key()] = false; generator.getInUseKeyCount().fetch_sub(1); });
-        return {start_key.key(), end_key.key()};
+        std::optional< std::pair< uint32_t, uint32_t > > ret = std::nullopt;
+        if (range_opt) {
+            ret = std::make_pair(range_opt->first.key(), range_opt->second.key());
+        }
+        return ret;
     }
 
     void long_running_crash(long_running_crash_options const& crash_test_options) {
@@ -740,8 +744,10 @@ struct IndexCrashTest : public test_common::HSTestHelper, BtreeTestHelper< TestT
             if (crash_test_options.range_remove_) {
                 // add one range remove operation
                 range_remove_keys = this->range_remove_op(generator, crash_test_options.num_entries_per_rounds);
-                LOGDEBUG("Range removing keys [{}, {})", range_remove_keys->first, range_remove_keys->second);
-                this->range_remove_all(range_remove_keys->first, range_remove_keys->second);
+                if (range_remove_keys) {
+                    LOGDEBUG("Range removing keys [{}, {})", range_remove_keys->first, range_remove_keys->second);
+                    this->range_remove_all(range_remove_keys->first, range_remove_keys->second);
+                }
             }
             if (normal_execution) {
                 if (clean_shutdown) {