Fix error and optimize

realm · Oct 19, 2023 · 8b3028e · 8b3028e
1 parent d8b9413
commit 8b3028e
Show file tree

Hide file tree

Showing 8 changed files with 45 additions and 25 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,7 +6,7 @@
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
-* Deleting an object in an asymmetric table would cause a crash. Likely to solve ["1537](https://github.com/realm/realm-kotlin/issues/1537), since v12.1.0.
+* Deleting an object in an asymmetric table would cause a crash. Likely to solve [#1537](https://github.com/realm/realm-kotlin/issues/1537), since v12.1.0.
 
 ### Breaking changes
 * None.

diff --git a/src/realm/cluster_tree.cpp b/src/realm/cluster_tree.cpp
@@ -1086,7 +1086,7 @@ void ClusterTree::verify() const
 #endif
 }
 
-void ClusterTree::nullify_links(ObjKey obj_key, CascadeState& state)
+void ClusterTree::nullify_incoming_links(ObjKey obj_key, CascadeState& state)
 {
     REALM_ASSERT(state.m_group);
     m_root->nullify_incoming_links(obj_key, state);

diff --git a/src/realm/cluster_tree.hpp b/src/realm/cluster_tree.hpp
@@ -72,7 +72,7 @@ class ClusterTree {
     {
         m_root->destroy_deep();
     }
-    void nullify_links(ObjKey, CascadeState&);
+    void nullify_incoming_links(ObjKey, CascadeState&);
     bool is_empty() const noexcept
     {
         return size() == 0;

diff --git a/src/realm/db.cpp b/src/realm/db.cpp
@@ -2412,10 +2412,15 @@ Replication::version_type DB::do_commit(Transaction& transaction, bool commit_to
     version_type new_version = current_version + 1;
 
     if (!transaction.m_objects_to_delete.empty()) {
+        std::map<TableKey, std::vector<ObjKey>> table_object_map;
         for (auto it : transaction.m_objects_to_delete) {
-            transaction.get_table(it.table_key)->remove_object(it.obj_key);
+            // Checking ttl would go here
+            table_object_map[it.table_key].push_back(it.obj_key);
         }
         transaction.m_objects_to_delete.clear();
+        for (auto& item : table_object_map) {
+            transaction.get_table_unchecked(item.first)->batch_erase_objects(item.second);
+        }
     }
     if (Replication* repl = get_replication()) {
         // If Replication::prepare_commit() fails, then the entire transaction

diff --git a/src/realm/group.hpp b/src/realm/group.hpp
@@ -809,6 +809,7 @@ class Group : public ArrayParent {
                                    std::optional<size_t> read_lock_file_size = util::none,
                                    std::optional<uint_fast64_t> read_lock_version = util::none);
 
+    Table* get_table_unchecked(TableKey);
     size_t find_table_index(StringData name) const noexcept;
     TableKey ndx2key(size_t ndx) const;
     size_t key2ndx(TableKey key) const;
@@ -946,11 +947,16 @@ inline TableKey Group::find_table(StringData name) const noexcept
     return (ndx != npos) ? ndx2key(ndx) : TableKey{};
 }
 
+inline Table* Group::get_table_unchecked(TableKey key)
+{
+    auto ndx = key2ndx_checked(key);
+    return do_get_table(ndx); // Throws
+}
+
 inline TableRef Group::get_table(TableKey key)
 {
     check_attached();
-    auto ndx = key2ndx_checked(key);
-    Table* table = do_get_table(ndx); // Throws
+    Table* table = get_table_unchecked(key);
     return TableRef(table, table ? table->m_alloc.get_instance_version() : 0);
 }
 

diff --git a/src/realm/sync/subscriptions.cpp b/src/realm/sync/subscriptions.cpp
@@ -883,8 +883,11 @@ MutableSubscriptionSet SubscriptionStore::get_mutable_by_version(int64_t version
 {
     auto tr = m_db->start_write();
     auto sub_sets = tr->get_table(m_sub_set_table);
-    return MutableSubscriptionSet(weak_from_this(), std::move(tr),
-                                  sub_sets->get_object_with_primary_key(Mixed{version_id}));
+    auto obj = sub_sets->get_object_with_primary_key(Mixed{version_id});
+    if (!obj) {
+        throw KeyNotFound("Subscription set not found");
+    }
+    return MutableSubscriptionSet(weak_from_this(), std::move(tr), obj);
 }
 
 SubscriptionSet SubscriptionStore::get_by_version(int64_t version_id) const
@@ -897,15 +900,16 @@ SubscriptionSet SubscriptionStore::get_by_version_impl(int64_t version_id,
 {
     auto tr = m_db->start_frozen(db_version.value_or(VersionID{}));
     auto sub_sets = tr->get_table(m_sub_set_table);
-    try {
-        return SubscriptionSet(weak_from_this(), *tr, sub_sets->get_object_with_primary_key(Mixed{version_id}));
+    auto obj = sub_sets->get_object_with_primary_key(Mixed{version_id});
+    if (obj) {
+        return SubscriptionSet(weak_from_this(), *tr, obj);
     }
-    catch (const KeyNotFound&) {
+    else {
         std::lock_guard<std::mutex> lk(m_pending_notifications_mutex);
         if (version_id < m_min_outstanding_version) {
             return SubscriptionSet(weak_from_this(), version_id, SubscriptionSet::SupersededTag{});
         }
-        throw;
+        throw KeyNotFound("Subscription set not found");
     }
 }
 

diff --git a/src/realm/table.cpp b/src/realm/table.cpp
@@ -548,7 +548,7 @@ void Table::remove_recursive(CascadeState& cascade_state)
         cascade_state.send_notifications();
 
         for (auto& l : cascade_state.m_to_be_nullified) {
-            Obj obj = group->get_table(l.origin_table)->try_get_object(l.origin_key);
+            Obj obj = group->get_table_unchecked(l.origin_table)->try_get_object(l.origin_key);
             REALM_ASSERT_DEBUG(obj);
             if (obj) {
                 std::move(obj).nullify_link(l.origin_col_key, l.old_target_link);
@@ -558,7 +558,7 @@ void Table::remove_recursive(CascadeState& cascade_state)
 
         auto to_delete = std::move(cascade_state.m_to_be_deleted);
         for (auto obj : to_delete) {
-            auto table = group->get_table(obj.first);
+            auto table = obj.first == m_key ? this : group->get_table_unchecked(obj.first);
             // This might add to the list of objects that should be deleted
             REALM_ASSERT(!obj.second.is_unresolved());
             table->m_clusters.erase(obj.second, cascade_state);
@@ -572,8 +572,9 @@ void Table::nullify_links(CascadeState& cascade_state)
     Group* group = get_parent_group();
     REALM_ASSERT(group);
     for (auto& to_delete : cascade_state.m_to_be_deleted) {
-        auto table = group->get_table(to_delete.first);
-        table->m_clusters.nullify_links(to_delete.second, cascade_state);
+        auto table = to_delete.first == m_key ? this : group->get_table_unchecked(to_delete.first);
+        if (!table->is_asymmetric())
+            table->m_clusters.nullify_incoming_links(to_delete.second, cascade_state);
     }
 }
 
@@ -2178,20 +2179,22 @@ void Table::batch_erase_rows(const KeyColumn& keys)
 void Table::batch_erase_objects(std::vector<ObjKey>& keys)
 {
     Group* g = get_parent_group();
+    bool maybe_has_incoming_links = g && !is_asymmetric();
 
     if (has_any_embedded_objects() || (g && g->has_cascade_notification_handler())) {
         CascadeState state(CascadeState::Mode::Strong, g);
         std::for_each(keys.begin(), keys.end(), [this, &state](ObjKey k) {
             state.m_to_be_deleted.emplace_back(m_key, k);
         });
-        nullify_links(state);
+        if (maybe_has_incoming_links)
+            nullify_links(state);
         remove_recursive(state);
     }
     else {
         CascadeState state(CascadeState::Mode::None, g);
         for (auto k : keys) {
-            if (g) {
-                m_clusters.nullify_links(k, state);
+            if (maybe_has_incoming_links) {
+                m_clusters.nullify_incoming_links(k, state);
             }
             m_clusters.erase(k, state);
         }
@@ -3147,7 +3150,8 @@ Obj Table::get_object_with_primary_key(Mixed primary_key) const
     DataType type = DataType(primary_key_col.get_type());
     REALM_ASSERT((primary_key.is_null() && primary_key_col.get_attrs().test(col_attr_Nullable)) ||
                  primary_key.get_type() == type);
-    return m_clusters.get(m_index_accessors[primary_key_col.get_index().val]->find_first(primary_key));
+    ObjKey k = m_index_accessors[primary_key_col.get_index().val]->find_first(primary_key);
+    return k ? m_clusters.get(k) : Obj{};
 }
 
 Mixed Table::get_primary_key(ObjKey key) const
@@ -3399,13 +3403,13 @@ void Table::remove_object(ObjKey key)
     if (has_any_embedded_objects() || (g && g->has_cascade_notification_handler())) {
         CascadeState state(CascadeState::Mode::Strong, g);
         state.m_to_be_deleted.emplace_back(m_key, key);
-        m_clusters.nullify_links(key, state);
+        m_clusters.nullify_incoming_links(key, state);
         remove_recursive(state);
     }
     else {
         CascadeState state(CascadeState::Mode::None, g);
         if (g) {
-            m_clusters.nullify_links(key, state);
+            m_clusters.nullify_incoming_links(key, state);
         }
         m_clusters.erase(key, state);
     }
@@ -3957,7 +3961,7 @@ bool Table::has_any_embedded_objects()
         for_each_public_column([&](ColKey col_key) {
             auto target_table_key = get_opposite_table_key(col_key);
             if (target_table_key && is_link_type(col_key.get_type())) {
-                auto target_table = get_parent_group()->get_table(target_table_key);
+                auto target_table = get_parent_group()->get_table_unchecked(target_table_key);
                 if (target_table->is_embedded()) {
                     m_has_any_embedded_objects = true;
                     return IteratorControl::Stop; // early out
@@ -3995,7 +3999,7 @@ ColKey Table::find_or_add_backlink_column(ColKey origin_col_key, TableKey origin
         set_opposite_column(backlink_col_key, origin_table, origin_col_key);
 
         if (Replication* repl = get_repl())
-            repl->typed_link_change(get_parent_group()->get_table(origin_table).unchecked_ptr(), origin_col_key,
+            repl->typed_link_change(get_parent_group()->get_table_unchecked(origin_table), origin_col_key,
                                     m_key); // Throws
     }
 

diff --git a/src/realm/table.hpp b/src/realm/table.hpp
@@ -335,6 +335,8 @@ class Table {
     // - turns the object into a tombstone if links exist
     // - otherwise works just as remove_object()
     ObjKey invalidate_object(ObjKey key);
+    // Remove several objects
+    void batch_erase_objects(std::vector<ObjKey>& keys);
     Obj try_get_tombstone(ObjKey key) const
     {
         REALM_ASSERT(key.is_unresolved());
@@ -703,7 +705,6 @@ class Table {
     TableRef m_own_ref;
 
     void batch_erase_rows(const KeyColumn& keys);
-    void batch_erase_objects(std::vector<ObjKey>& keys);
     size_t do_set_link(ColKey col_key, size_t row_ndx, size_t target_row_ndx);
 
     void populate_search_index(ColKey col_key);