From d8b941334117ac195779ef2195a4566515c692ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Mon, 16 Oct 2023 13:37:23 +0200 Subject: [PATCH 1/6] Allow deletion of ephemeral objects before commit --- CHANGELOG.md | 2 +- src/realm/table.cpp | 11 +++++++++++ test/object-store/object.cpp | 9 +++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ff5008fec0..dc4854abae3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* None. +* 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/table.cpp b/src/realm/table.cpp index 4aed1e4e636..4e12f8b8ab6 100644 --- a/src/realm/table.cpp +++ b/src/realm/table.cpp @@ -3385,6 +3385,17 @@ void Table::remove_object(ObjKey key) { Group* g = get_parent_group(); + if (is_asymmetric()) { + REALM_ASSERT(g); + auto it = std::find_if(g->m_objects_to_delete.begin(), g->m_objects_to_delete.end(), + [&](const Group::ToDeleteRef& item) { + return item.table_key == m_key && item.obj_key == key; + }); + if (it != g->m_objects_to_delete.end()) { + g->m_objects_to_delete.erase(it); + } + } + 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); diff --git a/test/object-store/object.cpp b/test/object-store/object.cpp index 015c452b3b1..6b237a2a6fb 100644 --- a/test/object-store/object.cpp +++ b/test/object-store/object.cpp @@ -2390,6 +2390,15 @@ TEST_CASE("Asymmetric Object") { REQUIRE(realm->is_empty()); } + SECTION("Delete ephemeral object before comitting") { + realm->begin_transaction(); + auto obj = realm->read_group().get_table("class_asymmetric")->create_object_with_primary_key(1); + obj.remove(); + realm->commit_transaction(); + REQUIRE(!obj.is_valid()); + REQUIRE(realm->is_empty()); + } + SECTION("Outgoing link not allowed") { auto obj = create(AnyDict{{"_id", INT64_C(1)}}, "table"); auto table = realm->read_group().get_table("class_table"); From 8b3028e8eafdb3972d888268e3b64e1b473e9b3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Wed, 18 Oct 2023 17:07:10 +0200 Subject: [PATCH 2/6] Fix error and optimize --- CHANGELOG.md | 2 +- src/realm/cluster_tree.cpp | 2 +- src/realm/cluster_tree.hpp | 2 +- src/realm/db.cpp | 7 ++++++- src/realm/group.hpp | 10 ++++++++-- src/realm/sync/subscriptions.cpp | 16 ++++++++++------ src/realm/table.cpp | 28 ++++++++++++++++------------ src/realm/table.hpp | 3 ++- 8 files changed, 45 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc4854abae3..0506a59a67a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Fixed * ([#????](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 index d7f65319ff6..f693a970281 100644 --- 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 index c61372dbe4f..9ebaf243775 100644 --- 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 index f916e6cf0df..77b35f8f498 100644 --- 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> 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 index b61bfe06782..0501742aed6 100644 --- a/src/realm/group.hpp +++ b/src/realm/group.hpp @@ -809,6 +809,7 @@ class Group : public ArrayParent { std::optional read_lock_file_size = util::none, std::optional 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 index 5585d5d742e..d8dffbce84a 100644 --- 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 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 index 4e12f8b8ab6..0f1fbed31ae 100644 --- 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& 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 index d248af3d57d..2a6d8b0c608 100644 --- 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& 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& 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); From dbb928bec7573abf29ac1fa901206d1d66b8b8f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Thu, 19 Oct 2023 10:23:03 +0200 Subject: [PATCH 3/6] Modify test --- test/object-store/sync/flx_sync.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/object-store/sync/flx_sync.cpp b/test/object-store/sync/flx_sync.cpp index 48132b14231..06133d357a7 100644 --- a/test/object-store/sync/flx_sync.cpp +++ b/test/object-store/sync/flx_sync.cpp @@ -2340,6 +2340,8 @@ TEST_CASE("flx: writes work without waiting for sync", "[sync][flx][baas]") { realm->refresh(); Results results(realm, table); CHECK(results.size() == 1); + Obj obj = results.get(0); + CHECK(obj.get_primary_key().get_object_id() == bar_obj_id); CHECK(table->get_object_with_primary_key({bar_obj_id}).is_valid()); }); } From 1042cef08cee3bb50f3bb49c44a90fc400190568 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Thu, 19 Oct 2023 13:11:30 +0200 Subject: [PATCH 4/6] Review update --- src/realm/sync/subscriptions.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/realm/sync/subscriptions.cpp b/src/realm/sync/subscriptions.cpp index d8dffbce84a..e0600bc1048 100644 --- a/src/realm/sync/subscriptions.cpp +++ b/src/realm/sync/subscriptions.cpp @@ -885,7 +885,7 @@ MutableSubscriptionSet SubscriptionStore::get_mutable_by_version(int64_t version auto sub_sets = tr->get_table(m_sub_set_table); auto obj = sub_sets->get_object_with_primary_key(Mixed{version_id}); if (!obj) { - throw KeyNotFound("Subscription set not found"); + throw KeyNotFound(util::format("Subscription set with version %1 not found", version_id)); } return MutableSubscriptionSet(weak_from_this(), std::move(tr), obj); } @@ -909,7 +909,7 @@ SubscriptionSet SubscriptionStore::get_by_version_impl(int64_t version_id, if (version_id < m_min_outstanding_version) { return SubscriptionSet(weak_from_this(), version_id, SubscriptionSet::SupersededTag{}); } - throw KeyNotFound("Subscription set not found"); + throw KeyNotFound(util::format("Subscription set with version %1 not found", version_id)); } } From 2b8ba962b80a98c62413c59a189fadacb1985176 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Thu, 19 Oct 2023 13:29:42 +0200 Subject: [PATCH 5/6] Optimize further by dropping ttl on ephemeral objects --- src/realm/db.cpp | 10 +++------- src/realm/group.hpp | 14 +------------- src/realm/table.cpp | 12 +++++------- 3 files changed, 9 insertions(+), 27 deletions(-) diff --git a/src/realm/db.cpp b/src/realm/db.cpp index 77b35f8f498..cfc53ec54ae 100644 --- a/src/realm/db.cpp +++ b/src/realm/db.cpp @@ -2412,15 +2412,11 @@ 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> table_object_map; - for (auto it : transaction.m_objects_to_delete) { - // Checking ttl would go here - table_object_map[it.table_key].push_back(it.obj_key); + for (auto& [table_key, obj_keys] : transaction.m_objects_to_delete) { + sort(obj_keys.begin(), obj_keys.end()); + transaction.get_table_unchecked(table_key)->batch_erase_objects(obj_keys); } 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 index 0501742aed6..3b7e1ed8b7e 100644 --- a/src/realm/group.hpp +++ b/src/realm/group.hpp @@ -538,18 +538,6 @@ class Group : public ArrayParent { private: static constexpr StringData g_class_name_prefix = "class_"; - struct ToDeleteRef { - ToDeleteRef(TableKey tk, ObjKey k) - : table_key(tk) - , obj_key(k) - , ttl(std::chrono::steady_clock::now()) - { - } - TableKey table_key; - ObjKey obj_key; - std::chrono::steady_clock::time_point ttl; - }; - // nullptr, if we're sharing an allocator provided during initialization std::unique_ptr m_local_alloc; // in-use allocator. If local, then equal to m_local_alloc. @@ -614,7 +602,7 @@ class Group : public ArrayParent { util::UniqueFunction m_notify_handler; util::UniqueFunction m_schema_change_handler; - std::vector m_objects_to_delete; + std::map> m_objects_to_delete; Group(SlabAlloc* alloc) noexcept; void init_array_parents() noexcept; diff --git a/src/realm/table.cpp b/src/realm/table.cpp index 0f1fbed31ae..73afcec9230 100644 --- a/src/realm/table.cpp +++ b/src/realm/table.cpp @@ -3056,7 +3056,7 @@ Obj Table::create_object_with_primary_key(const Mixed& primary_key, FieldValues& } } if (is_asymmetric() && repl && repl->get_history_type() == Replication::HistoryType::hist_SyncClient) { - get_parent_group()->m_objects_to_delete.emplace_back(this->m_key, ret.get_key()); + get_parent_group()->m_objects_to_delete[this->m_key].emplace_back(ret.get_key()); } return ret; } @@ -3391,12 +3391,10 @@ void Table::remove_object(ObjKey key) if (is_asymmetric()) { REALM_ASSERT(g); - auto it = std::find_if(g->m_objects_to_delete.begin(), g->m_objects_to_delete.end(), - [&](const Group::ToDeleteRef& item) { - return item.table_key == m_key && item.obj_key == key; - }); - if (it != g->m_objects_to_delete.end()) { - g->m_objects_to_delete.erase(it); + auto& obj_keys = g->m_objects_to_delete[m_key]; + auto it = std::find(obj_keys.begin(), obj_keys.end(), key); + if (it != obj_keys.end()) { + obj_keys.erase(it); } } From fc03907d3f6735b6fa66f0f4cf498517fc0f993d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Thu, 19 Oct 2023 15:34:56 +0200 Subject: [PATCH 6/6] Just clear asymmetric tables --- src/realm/db.cpp | 9 ++++----- src/realm/group.hpp | 4 ++-- src/realm/table.cpp | 11 +---------- 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/src/realm/db.cpp b/src/realm/db.cpp index cfc53ec54ae..9328c485dd9 100644 --- a/src/realm/db.cpp +++ b/src/realm/db.cpp @@ -2411,12 +2411,11 @@ 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()) { - for (auto& [table_key, obj_keys] : transaction.m_objects_to_delete) { - sort(obj_keys.begin(), obj_keys.end()); - transaction.get_table_unchecked(table_key)->batch_erase_objects(obj_keys); + if (!transaction.m_tables_to_clear.empty()) { + for (auto table_key : transaction.m_tables_to_clear) { + transaction.get_table_unchecked(table_key)->clear(); } - transaction.m_objects_to_delete.clear(); + transaction.m_tables_to_clear.clear(); } 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 index 3b7e1ed8b7e..eafd76d5d38 100644 --- a/src/realm/group.hpp +++ b/src/realm/group.hpp @@ -24,7 +24,7 @@ #include #include #include -#include +#include #include #include @@ -602,7 +602,7 @@ class Group : public ArrayParent { util::UniqueFunction m_notify_handler; util::UniqueFunction m_schema_change_handler; - std::map> m_objects_to_delete; + std::set m_tables_to_clear; Group(SlabAlloc* alloc) noexcept; void init_array_parents() noexcept; diff --git a/src/realm/table.cpp b/src/realm/table.cpp index 73afcec9230..6eb6199fd73 100644 --- a/src/realm/table.cpp +++ b/src/realm/table.cpp @@ -3056,7 +3056,7 @@ Obj Table::create_object_with_primary_key(const Mixed& primary_key, FieldValues& } } if (is_asymmetric() && repl && repl->get_history_type() == Replication::HistoryType::hist_SyncClient) { - get_parent_group()->m_objects_to_delete[this->m_key].emplace_back(ret.get_key()); + get_parent_group()->m_tables_to_clear.insert(this->m_key); } return ret; } @@ -3389,15 +3389,6 @@ void Table::remove_object(ObjKey key) { Group* g = get_parent_group(); - if (is_asymmetric()) { - REALM_ASSERT(g); - auto& obj_keys = g->m_objects_to_delete[m_key]; - auto it = std::find(obj_keys.begin(), obj_keys.end(), key); - if (it != obj_keys.end()) { - obj_keys.erase(it); - } - } - 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);