From fe1d852598d6d9da70404a5551cb11f7607ca4e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Thu, 12 Jan 2023 10:29:45 +0100 Subject: [PATCH 1/2] Remove ability to synchronize objects without primary key With the legacy server this was based on GlobalKey, but if we at some point will allow objects without primary key to be synced to MongoDB server, we will probably have to invent a new strategy. As we remove support for GlobalKey we will have to change the way we allocate ObjKeys for tombstones. Beforehand we used a hash of the primary key to construct a GlobalKey and from that an ObjKey. Now we just use the current ObjKey to construct a corresponding unresolved key. But it has the effect that there is not an easy way to come from primary key to potential unresolved key, so we will have to search for a potential tombstone. --- src/realm/cluster_tree.cpp | 1 - src/realm/obj.cpp | 5 - src/realm/obj.hpp | 2 - src/realm/replication.cpp | 4 +- src/realm/replication.hpp | 2 +- src/realm/sync/instruction_applier.cpp | 18 +- src/realm/sync/instruction_replication.cpp | 28 +- src/realm/sync/instruction_replication.hpp | 1 - src/realm/table.cpp | 352 ++---------------- src/realm/table.hpp | 37 +- src/realm/transaction.cpp | 7 + test/CMakeLists.txt | 2 - test/object-store/transaction_log_parsing.cpp | 14 +- test/test_global_key.cpp | 77 ---- test/test_mixed_null_assertions.cpp | 50 +-- test/test_set.cpp | 18 +- test/test_stable_ids.cpp | 322 ---------------- test/test_sync.cpp | 43 --- test/test_unresolved_links.cpp | 19 +- test/util/compare_groups.cpp | 11 +- 20 files changed, 107 insertions(+), 906 deletions(-) delete mode 100644 test/test_global_key.cpp delete mode 100644 test/test_stable_ids.cpp diff --git a/src/realm/cluster_tree.cpp b/src/realm/cluster_tree.cpp index 23390a67ffe..8ae6fe211c8 100644 --- a/src/realm/cluster_tree.cpp +++ b/src/realm/cluster_tree.cpp @@ -994,7 +994,6 @@ void ClusterTree::erase(ObjKey k, CascadeState& state) } } } - m_owner->free_local_id_after_hash_collision(k); m_owner->erase_from_search_indexes(k); size_t root_size = m_root->erase(k, state); diff --git a/src/realm/obj.cpp b/src/realm/obj.cpp index a430e798fc8..2ea44217146 100644 --- a/src/realm/obj.cpp +++ b/src/realm/obj.cpp @@ -114,11 +114,6 @@ Obj::Obj(TableRef table, MemRef mem, ObjKey key, size_t row_ndx) m_storage_version = get_alloc().get_storage_version(); } -GlobalKey Obj::get_object_id() const -{ - return m_table->get_object_id(m_key); -} - ObjLink Obj::get_link() const { return ObjLink(m_table->get_key(), m_key); diff --git a/src/realm/obj.hpp b/src/realm/obj.hpp index 4db877aac95..6ef74bda065 100644 --- a/src/realm/obj.hpp +++ b/src/realm/obj.hpp @@ -34,7 +34,6 @@ class ClusterTree; class TableView; class CascadeState; class ObjList; -struct GlobalKey; template class Lst; @@ -114,7 +113,6 @@ class Obj : public CollectionParent { { return m_key; } - GlobalKey get_object_id() const; ObjLink get_link() const; /// Check if the object is still alive diff --git a/src/realm/replication.cpp b/src/realm/replication.cpp index a230d29a798..9c84ecf714e 100644 --- a/src/realm/replication.cpp +++ b/src/realm/replication.cpp @@ -139,13 +139,13 @@ void Replication::erase_column(const Table* t, ColKey col_key) m_encoder.erase_column(col_key); // Throws } -void Replication::create_object(const Table* t, GlobalKey id) +void Replication::create_object(const Table* t, ObjKey key) { if (auto logger = get_logger()) { logger->log(util::Logger::Level::debug, "Create object '%1'", t->get_class_name()); } select_table(t); // Throws - m_encoder.create_object(id.get_local_key(0)); // Throws + m_encoder.create_object(key); // Throws } void Replication::create_object_with_primary_key(const Table* t, ObjKey key, Mixed pk) diff --git a/src/realm/replication.hpp b/src/realm/replication.hpp index 437d6654901..8e315ab14b9 100644 --- a/src/realm/replication.hpp +++ b/src/realm/replication.hpp @@ -74,7 +74,7 @@ class Replication { virtual void dictionary_erase(const CollectionBase& dict, size_t dict_ndx, Mixed key); virtual void dictionary_clear(const CollectionBase& dict); - virtual void create_object(const Table*, GlobalKey); + virtual void create_object(const Table*, ObjKey); virtual void create_object_with_primary_key(const Table*, ObjKey, Mixed); virtual void remove_object(const Table*, ObjKey); diff --git a/src/realm/sync/instruction_applier.cpp b/src/realm/sync/instruction_applier.cpp index 98f29bcdc28..c89fce1cae4 100644 --- a/src/realm/sync/instruction_applier.cpp +++ b/src/realm/sync/instruction_applier.cpp @@ -226,11 +226,8 @@ void InstructionApplier::operator()(const Instruction::CreateObject& instr) } m_last_object = table->create_object_with_primary_key(id); }, - [&](GlobalKey key) { - if (pk_col) { - bad_transaction_log("CreateObject(GlobalKey) on table with a primary key"); - } - m_last_object = table->create_object(key); + [&](GlobalKey) { + bad_transaction_log("CreateObject(GlobalKey) not supported"); }, }, instr.object); @@ -1682,14 +1679,9 @@ ObjKey InstructionApplier::get_object_key(Table& table, const Instruction::Prima ObjKey key = table.get_objkey_from_primary_key(pk); return key; }, - [&](GlobalKey id) { - if (pk_col) { - bad_transaction_log( - "%1 instruction without primary key, but table '%2' has a primary key column of type %3", - name, table_name, pk_type); - } - ObjKey key = table.get_objkey_from_global_key(id); - return key; + [&](GlobalKey) { + bad_transaction_log("%1 instruction without primary key not supported", name); + return ObjKey(); }, [&](ObjectId pk) { if (!pk_col) { diff --git a/src/realm/sync/instruction_replication.cpp b/src/realm/sync/instruction_replication.cpp index ef3c17c1701..00d61537ad3 100644 --- a/src/realm/sync/instruction_replication.cpp +++ b/src/realm/sync/instruction_replication.cpp @@ -242,26 +242,6 @@ void SyncReplication::add_class_with_primary_key(TableKey tk, StringData name, D } } -void SyncReplication::create_object(const Table* table, GlobalKey oid) -{ - if (table->is_embedded()) { - unsupported_instruction(); // FIXME: TODO - } - - Replication::create_object(table, oid); - if (select_table(*table)) { - if (table->get_primary_key_column()) { - // Trying to create object without a primary key in a table that - // has a primary key column. - unsupported_instruction(); - } - Instruction::CreateObject instr; - instr.table = m_last_class_name; - instr.object = oid; - emit(instr); - } -} - Instruction::PrimaryKey SyncReplication::as_primary_key(Mixed value) { if (value.is_null()) { @@ -749,13 +729,7 @@ Instruction::PrimaryKey SyncReplication::primary_key_for_object(const Table& tab { bool should_emit = select_table(table); REALM_ASSERT(should_emit); - - if (table.get_primary_key_column()) { - return as_primary_key(table.get_primary_key(key)); - } - - GlobalKey global_key = table.get_object_id(key); - return global_key; + return as_primary_key(table.get_primary_key(key)); } void SyncReplication::populate_path_instr(Instruction::PathInstruction& instr, const Table& table, ObjKey key, diff --git a/src/realm/sync/instruction_replication.hpp b/src/realm/sync/instruction_replication.hpp index b9008b9609b..146a57e774d 100644 --- a/src/realm/sync/instruction_replication.hpp +++ b/src/realm/sync/instruction_replication.hpp @@ -48,7 +48,6 @@ class SyncReplication : public Replication { void add_class(TableKey tk, StringData table_name, Table::Type table_type = Table::Type::TopLevel) final; void add_class_with_primary_key(TableKey tk, StringData table_name, DataType pk_type, StringData pk_field, bool nullable, Table::Type table_type) final; - void create_object(const Table*, GlobalKey) final; void create_object_with_primary_key(const Table*, ObjKey, Mixed) final; void erase_class(TableKey table_key, StringData table_name, size_t num_tables) final; diff --git a/src/realm/table.cpp b/src/realm/table.cpp index 46ca75f2f1e..ff0bae7e6c1 100644 --- a/src/realm/table.cpp +++ b/src/realm/table.cpp @@ -1448,7 +1448,6 @@ void Table::clear() { CascadeState state(CascadeState::Mode::Strong, get_parent_group()); m_clusters.clear(state); - free_collision_table(); } @@ -1494,11 +1493,6 @@ void Table::set_sequence_number(uint64_t seq) m_top.set(top_position_for_sequence_number, RefOrTagged::make_tagged(seq)); } -void Table::set_collision_map(ref_type ref) -{ - m_top.set(top_position_for_collision_map, RefOrTagged::make_ref(ref)); -} - TableRef Table::get_link_target(ColKey col_key) noexcept { return get_opposite_table(col_key); @@ -2141,17 +2135,9 @@ Obj Table::create_object(ObjKey key, const FieldValues& values) if (m_primary_key_col) throw IllegalOperation(util::format("Table has primary key: %1", get_name())); if (key == null_key) { - GlobalKey object_id = allocate_object_id_squeezed(); - key = object_id.get_local_key(get_sync_file_id()); - // Check if this key collides with an already existing object - // This could happen if objects were at some point created with primary keys, - // but later primary key property was removed from the schema. - while (m_clusters.is_valid(key)) { - object_id = allocate_object_id_squeezed(); - key = object_id.get_local_key(get_sync_file_id()); - } + key = get_next_valid_key(); if (auto repl = get_repl()) - repl->create_object(this, object_id); + repl->create_object(this, key); } REALM_ASSERT(key.value >= 0); @@ -2165,49 +2151,12 @@ Obj Table::create_linked_object() { REALM_ASSERT(is_embedded()); - GlobalKey object_id = allocate_object_id_squeezed(); - ObjKey key = object_id.get_local_key(get_sync_file_id()); - - REALM_ASSERT(key.value >= 0); - + ObjKey key = get_next_valid_key(); Obj obj = m_clusters.insert(key, {}); return obj; } -Obj Table::create_object(GlobalKey object_id, const FieldValues& values) -{ - if (is_embedded()) - throw IllegalOperation(util::format("Explicit creation of embedded object not allowed in: %1", get_name())); - if (m_primary_key_col) - throw IllegalOperation(util::format("Table has primary key: %1", get_name())); - ObjKey key = object_id.get_local_key(get_sync_file_id()); - - if (auto repl = get_repl()) - repl->create_object(this, object_id); - - try { - Obj obj = m_clusters.insert(key, values); - // Check if tombstone exists - if (m_tombstones && m_tombstones->is_valid(key.get_unresolved())) { - auto unres_key = key.get_unresolved(); - // Copy links over - auto tombstone = m_tombstones->get(unres_key); - obj.assign_pk_and_backlinks(tombstone); - // If tombstones had no links to it, it may still be alive - if (m_tombstones->is_valid(unres_key)) { - CascadeState state(CascadeState::Mode::None); - m_tombstones->erase(unres_key, state); - } - } - - return obj; - } - catch (const KeyAlreadyUsed&) { - return m_clusters.get(key); - } -} - Obj Table::create_object_with_primary_key(const Mixed& primary_key, FieldValues&& field_values, UpdateMode mode, bool* did_create) { @@ -2251,17 +2200,16 @@ Obj Table::create_object_with_primary_key(const Mixed& primary_key, FieldValues& ObjKey unres_key; if (m_tombstones) { - // Check for potential tombstone - GlobalKey object_id{primary_key}; - ObjKey object_key = global_to_local_object_id_hashed(object_id); - - ObjKey key = object_key.get_unresolved(); - if (auto obj = m_tombstones->try_get_obj(key)) { - auto existing_pk_value = obj.get_any(primary_key_col); - - // If the primary key is the same, the object should be resurrected below - if (existing_pk_value == primary_key) { - unres_key = key; + if (auto sz = m_tombstones->size()) { + // Check for potential tombstone + Iterator end(*m_tombstones, sz); + for (Iterator it(*m_tombstones, 0); it != end; ++it) { + auto existing_pk_value = it->get_any(primary_key_col); + // If the primary key is the same, the object should be resurrected below + if (existing_pk_value == primary_key) { + unres_key = it->get_key(); + break; + } } } } @@ -2302,25 +2250,9 @@ ObjKey Table::find_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); + REALM_ASSERT(m_index_accessors[primary_key_col.get_index().val]); - if (auto&& index = m_index_accessors[primary_key_col.get_index().val]) { - return index->find_first(primary_key); - } - - // This must be file format 11, 20 or 21 as those are the ones we can open in read-only mode - // so try the old algorithm - GlobalKey object_id{primary_key}; - ObjKey object_key = global_to_local_object_id_hashed(object_id); - - // Check if existing - if (auto obj = m_clusters.try_get_obj(object_key)) { - auto existing_pk_value = obj.get_any(primary_key_col); - - if (existing_pk_value == primary_key) { - return object_key; - } - } - return {}; + return m_index_accessors[primary_key_col.get_index().val]->find_first(primary_key); } ObjKey Table::get_objkey_from_primary_key(const Mixed& primary_key) @@ -2330,51 +2262,7 @@ ObjKey Table::get_objkey_from_primary_key(const Mixed& primary_key) return key; } - // Object does not exist - create tombstone - GlobalKey object_id{primary_key}; - ObjKey object_key = global_to_local_object_id_hashed(object_id); - return get_or_create_tombstone(object_key, m_primary_key_col, primary_key).get_key(); -} - -ObjKey Table::get_objkey_from_global_key(GlobalKey global_key) -{ - REALM_ASSERT(!m_primary_key_col); - auto object_key = global_key.get_local_key(get_sync_file_id()); - - // Check if existing - if (m_clusters.is_valid(object_key)) { - return object_key; - } - - return get_or_create_tombstone(object_key, {}, {}).get_key(); -} - -ObjKey Table::get_objkey(GlobalKey global_key) const -{ - ObjKey key; - REALM_ASSERT(!m_primary_key_col); - uint32_t max = std::numeric_limits::max(); - if (global_key.hi() <= max && global_key.lo() <= max) { - key = global_key.get_local_key(get_sync_file_id()); - } - if (key && !is_valid(key)) { - key = realm::null_key; - } - return key; -} - -GlobalKey Table::get_object_id(ObjKey key) const -{ - auto col = get_primary_key_column(); - if (col) { - const Obj obj = get_object(key); - auto val = obj.get_any(col); - return {val}; - } - else { - return {key, get_sync_file_id()}; - } - return {}; + return get_or_create_tombstone(ObjKey{}, m_primary_key_col, primary_key).get_key(); } Obj Table::get_object_with_primary_key(Mixed primary_key) const @@ -2400,193 +2288,28 @@ Mixed Table::get_primary_key(ObjKey key) const } } -GlobalKey Table::allocate_object_id_squeezed() -{ - // m_client_file_ident will be zero if we haven't been in contact with - // the server yet. - auto peer_id = get_sync_file_id(); - auto sequence = allocate_sequence_number(); - return GlobalKey{peer_id, sequence}; -} - -namespace { - -/// Calculate optimistic local ID that may collide with others. It is up to -/// the caller to ensure that collisions are detected and that -/// allocate_local_id_after_collision() is called to obtain a non-colliding -/// ID. -inline ObjKey get_optimistic_local_id_hashed(GlobalKey global_id) -{ -#if REALM_EXERCISE_OBJECT_ID_COLLISION - const uint64_t optimistic_mask = 0xff; -#else - const uint64_t optimistic_mask = 0x3fffffffffffffff; -#endif - static_assert(!(optimistic_mask >> 62), "optimistic Object ID mask must leave the 63rd and 64th bit zero"); - return ObjKey{int64_t(global_id.lo() & optimistic_mask)}; -} - -inline ObjKey make_tagged_local_id_after_hash_collision(uint64_t sequence_number) -{ - REALM_ASSERT(!(sequence_number >> 62)); - return ObjKey{int64_t(0x4000000000000000 | sequence_number)}; -} - -} // namespace - -ObjKey Table::global_to_local_object_id_hashed(GlobalKey object_id) const -{ - ObjKey optimistic = get_optimistic_local_id_hashed(object_id); - - if (ref_type collision_map_ref = to_ref(m_top.get(top_position_for_collision_map))) { - Allocator& alloc = m_top.get_alloc(); - Array collision_map{alloc}; - collision_map.init_from_ref(collision_map_ref); // Throws - - Array hi{alloc}; - hi.init_from_ref(to_ref(collision_map.get(s_collision_map_hi))); // Throws - - // Entries are ordered by hi,lo - size_t found = hi.find_first(object_id.hi()); - if (found != npos && uint64_t(hi.get(found)) == object_id.hi()) { - Array lo{alloc}; - lo.init_from_ref(to_ref(collision_map.get(s_collision_map_lo))); // Throws - size_t candidate = lo.find_first(object_id.lo(), found); - if (candidate != npos && uint64_t(hi.get(candidate)) == object_id.hi()) { - Array local_id{alloc}; - local_id.init_from_ref(to_ref(collision_map.get(s_collision_map_local_id))); // Throws - return ObjKey{local_id.get(candidate)}; - } - } - } - - return optimistic; -} - -ObjKey Table::allocate_local_id_after_hash_collision(GlobalKey incoming_id, GlobalKey colliding_id, - ObjKey colliding_local_id) -{ - // Possible optimization: Cache these accessors - Allocator& alloc = m_top.get_alloc(); - Array collision_map{alloc}; - Array hi{alloc}; - Array lo{alloc}; - Array local_id{alloc}; - - collision_map.set_parent(&m_top, top_position_for_collision_map); - hi.set_parent(&collision_map, s_collision_map_hi); - lo.set_parent(&collision_map, s_collision_map_lo); - local_id.set_parent(&collision_map, s_collision_map_local_id); - - ref_type collision_map_ref = to_ref(m_top.get(top_position_for_collision_map)); - if (collision_map_ref) { - collision_map.init_from_parent(); // Throws - } - else { - MemRef mem = Array::create_empty_array(Array::type_HasRefs, false, alloc); // Throws - collision_map.init_from_mem(mem); // Throws - collision_map.update_parent(); - - ref_type lo_ref = Array::create_array(Array::type_Normal, false, 0, 0, alloc).get_ref(); // Throws - ref_type hi_ref = Array::create_array(Array::type_Normal, false, 0, 0, alloc).get_ref(); // Throws - ref_type local_id_ref = Array::create_array(Array::type_Normal, false, 0, 0, alloc).get_ref(); // Throws - collision_map.add(lo_ref); // Throws - collision_map.add(hi_ref); // Throws - collision_map.add(local_id_ref); // Throws - } - - hi.init_from_parent(); // Throws - lo.init_from_parent(); // Throws - local_id.init_from_parent(); // Throws - - size_t num_entries = hi.size(); - REALM_ASSERT(lo.size() == num_entries); - REALM_ASSERT(local_id.size() == num_entries); - - auto lower_bound_object_id = [&](GlobalKey object_id) -> size_t { - size_t i = hi.lower_bound_int(int64_t(object_id.hi())); - while (i < num_entries && uint64_t(hi.get(i)) == object_id.hi() && uint64_t(lo.get(i)) < object_id.lo()) - ++i; - return i; - }; - - auto insert_collision = [&](GlobalKey object_id, ObjKey new_local_id) { - size_t i = lower_bound_object_id(object_id); - if (i != num_entries) { - GlobalKey existing{uint64_t(hi.get(i)), uint64_t(lo.get(i))}; - if (existing == object_id) { - REALM_ASSERT(new_local_id.value == local_id.get(i)); - return; - } - } - hi.insert(i, int64_t(object_id.hi())); - lo.insert(i, int64_t(object_id.lo())); - local_id.insert(i, new_local_id.value); - ++num_entries; - }; - - auto sequence_number_for_local_id = allocate_sequence_number(); - ObjKey new_local_id = make_tagged_local_id_after_hash_collision(sequence_number_for_local_id); - insert_collision(incoming_id, new_local_id); - insert_collision(colliding_id, colliding_local_id); - - return new_local_id; -} - Obj Table::get_or_create_tombstone(ObjKey key, ColKey pk_col, Mixed pk_val) { - auto unres_key = key.get_unresolved(); - ensure_graveyard(); - auto tombstone = m_tombstones->try_get_obj(unres_key); - if (tombstone) { - if (pk_col) { - auto existing_pk_value = tombstone.get_any(pk_col); - // It may just be the same object - if (existing_pk_value != pk_val) { - // We have a collision - create new ObjKey - key = allocate_local_id_after_hash_collision({pk_val}, {existing_pk_value}, key); - return get_or_create_tombstone(key, pk_col, pk_val); + if (auto sz = m_tombstones->size()) { + // Check for existing tombstone + Iterator end(*m_tombstones, sz); + for (Iterator it(*m_tombstones, 0); it != end; ++it) { + auto existing_pk_value = it->get_any(m_primary_key_col); + // If the primary key is the same, the object should be resurrected below + if (existing_pk_value == pk_val) { + return *it; } } - return tombstone; } - return m_tombstones->insert(unres_key, {{pk_col, pk_val}}); -} -void Table::free_local_id_after_hash_collision(ObjKey key) -{ - if (ref_type collision_map_ref = to_ref(m_top.get(top_position_for_collision_map))) { - if (key.is_unresolved()) { - // Keys will always be inserted as resolved - key = key.get_unresolved(); - } - // Possible optimization: Cache these accessors - Array collision_map{m_alloc}; - Array local_id{m_alloc}; - - collision_map.set_parent(&m_top, top_position_for_collision_map); - local_id.set_parent(&collision_map, s_collision_map_local_id); - collision_map.init_from_ref(collision_map_ref); - local_id.init_from_parent(); - auto ndx = local_id.find_first(key.value); - if (ndx != realm::npos) { - Array hi{m_alloc}; - Array lo{m_alloc}; - - hi.set_parent(&collision_map, s_collision_map_hi); - lo.set_parent(&collision_map, s_collision_map_lo); - hi.init_from_parent(); - lo.init_from_parent(); - - hi.erase(ndx); - lo.erase(ndx); - local_id.erase(ndx); - if (hi.size() == 0) { - free_collision_table(); - } - } + // Create new tombstone + if (!key) { + key = get_next_valid_key(); } + auto unres_key = key.get_unresolved(); + REALM_ASSERT(!m_tombstones->is_valid(unres_key)); + return m_tombstones->insert(unres_key, {{pk_col, pk_val}}); } void Table::free_collision_table() @@ -2648,15 +2371,10 @@ ObjKey Table::invalidate_object(ObjKey key) if (obj.has_backlinks(false)) { // If the object has backlinks, we should make a tombstone // and make inward links point to it, - if (auto primary_key_col = get_primary_key_column()) { - auto pk = obj.get_any(primary_key_col); - GlobalKey object_id{pk}; - auto unres_key = global_to_local_object_id_hashed(object_id); - tombstone = get_or_create_tombstone(unres_key, primary_key_col, pk); - } - else { - tombstone = get_or_create_tombstone(key, {}, {}); - } + auto primary_key_col = get_primary_key_column(); + REALM_ASSERT(primary_key_col); + auto pk = obj.get_any(primary_key_col); + tombstone = get_or_create_tombstone(key, primary_key_col, pk); tombstone.assign_pk_and_backlinks(obj); } @@ -2892,7 +2610,7 @@ ObjKey Table::get_next_valid_key() ObjKey key; do { key = ObjKey(allocate_sequence_number()); - } while (m_clusters.is_valid(key)); + } while (key.value < m_clusters.get_last_key_value() && m_clusters.is_valid(key)); return key; } diff --git a/src/realm/table.hpp b/src/realm/table.hpp index ec9e03eb989..16d946cfb1e 100644 --- a/src/realm/table.hpp +++ b/src/realm/table.hpp @@ -35,7 +35,6 @@ #include #include #include -#include // Only set this to one when testing the code paths that exercise object ID // hash collisions. It artificially limits the "optimistic" local ID to use @@ -293,9 +292,6 @@ class Table { // Create an object with key. If the key is omitted, a key will be generated by the system Obj create_object(ObjKey key = {}, const FieldValues& = {}); - // Create an object with specific GlobalKey - or return already existing object - // Potential tombstone will be resurrected - Obj create_object(GlobalKey object_id, const FieldValues& = {}); // Create an object with primary key. If an object with the given primary key already exists, it // will be returned and did_create (if supplied) will be set to false. // Potential tombstone will be resurrected @@ -307,18 +303,10 @@ class Table { } // Return key for existing object or return null key. ObjKey find_primary_key(Mixed value) const; - // Return ObjKey for object identified by id. If objects does not exist, return null key - // Important: This function must not be called for tables with primary keys. - ObjKey get_objkey(GlobalKey id) const; // Return key for existing object or return unresolved key. // Important: This is to be used ONLY by the Sync client. SDKs should NEVER // observe an unresolved key. Ever. ObjKey get_objkey_from_primary_key(const Mixed& primary_key); - // Return key for existing object or return unresolved key. - // Important: This is to be used ONLY by the Sync client. SDKs should NEVER - // observe an unresolved key. Ever. - // Important (2): This function must not be called for tables with primary keys. - ObjKey get_objkey_from_global_key(GlobalKey key); /// Create a number of objects and add corresponding keys to a vector void create_objects(size_t number, std::vector& keys); /// Create a number of objects with keys supplied @@ -328,7 +316,6 @@ class Table { { return m_clusters.is_valid(key); } - GlobalKey get_object_id(ObjKey key) const; Obj get_object(ObjKey key) const { REALM_ASSERT(!key.is_unresolved()); @@ -415,7 +402,6 @@ class Table { uint64_t allocate_sequence_number(); // Used by upgrade void set_sequence_number(uint64_t seq); - void set_collision_map(ref_type ref); // Get the key of this table directly, without needing a Table accessor. static TableKey get_key_direct(Allocator& alloc, ref_type top_ref); @@ -785,26 +771,9 @@ class Table { void validate_column_is_unique(ColKey col_key) const; ObjKey get_next_valid_key(); - /// Some Object IDs are generated as a tuple of the client_file_ident and a - /// local sequence number. This function takes the next number in the - /// sequence for the given table and returns an appropriate globally unique - /// GlobalKey. - GlobalKey allocate_object_id_squeezed(); - - /// Find the local 64-bit object ID for the provided global 128-bit ID. - ObjKey global_to_local_object_id_hashed(GlobalKey global_id) const; - - /// After a local ObjKey collision has been detected, this function may be - /// called to obtain a non-colliding local ObjKey in such a way that subsequent - /// calls to global_to_local_object_id() will return the correct local ObjKey - /// for both \a incoming_id and \a colliding_id. - ObjKey allocate_local_id_after_hash_collision(GlobalKey incoming_id, GlobalKey colliding_id, - ObjKey colliding_local_id); /// Create a placeholder for a not yet existing object and return key to it Obj get_or_create_tombstone(ObjKey key, ColKey pk_col, Mixed pk_val); - /// Should be called when an object is deleted - void free_local_id_after_hash_collision(ObjKey key); - /// Should be called when last entry is removed - or when table is cleared + /// Should be called from upgrade function void free_collision_table(); /// Called in the context of Group::commit() to ensure that @@ -1420,10 +1389,6 @@ class _impl::TableFriend { { table.batch_erase_rows(keys); // Throws } - static ObjKey global_to_local_object_id_hashed(const Table& table, GlobalKey global_id) - { - return table.global_to_local_object_id_hashed(global_id); - } }; } // namespace realm diff --git a/src/realm/transaction.cpp b/src/realm/transaction.cpp index 47b80362d6d..03c15438fec 100644 --- a/src/realm/transaction.cpp +++ b/src/realm/transaction.cpp @@ -562,6 +562,13 @@ void Transaction::upgrade_file_format(int target_file_format_version) t->migrate_sets_and_dictionaries(); } } + if (current_file_format_version < 24) { + for (auto k : table_keys) { + auto t = get_table(k); + t->free_collision_table(); + } + } + // NOTE: Additional future upgrade steps go here. } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 75c7309e1c6..cdb37a1df95 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -46,7 +46,6 @@ set(CORE_TEST_SOURCES test_dictionary.cpp test_file.cpp test_file_locks.cpp - test_global_key.cpp test_group.cpp test_impl_simulated_failure.cpp test_index_string.cpp @@ -189,7 +188,6 @@ if(REALM_ENABLE_SYNC) test_instruction_replication.cpp test_lang_bind_helper_sync.cpp test_noinst_server_dir.cpp - test_stable_ids.cpp test_sync.cpp test_sync_auth.cpp test_sync_history_migration.cpp diff --git a/test/object-store/transaction_log_parsing.cpp b/test/object-store/transaction_log_parsing.cpp index 3de2cfc6e1a..cc534fe89c9 100644 --- a/test/object-store/transaction_log_parsing.cpp +++ b/test/object-store/transaction_log_parsing.cpp @@ -1592,7 +1592,11 @@ TEMPLATE_TEST_CASE("DeepChangeChecker collections", "[notifications]", cf::ListO auto r = Realm::get_shared_realm(config); r->update_schema({ {"table", - {{"int", PropertyType::Int}, + {{ + "int", + PropertyType::Int, + Property::IsPrimary{true}, + }, {"link1", PropertyType::Object | PropertyType::Nullable, "table"}, {"link2", PropertyType::Object | PropertyType::Nullable, "table"}, test_type.property()}}, @@ -1603,7 +1607,7 @@ TEMPLATE_TEST_CASE("DeepChangeChecker collections", "[notifications]", cf::ListO std::vector objects; r->begin_transaction(); for (int i = 0; i < 10; ++i) - objects.push_back(table->create_object().set_all(i)); + objects.push_back(table->create_object_with_primary_key(i)); r->commit_transaction(); auto track_changes = [&](auto&& f) { @@ -1835,7 +1839,7 @@ TEST_CASE("DeepChangeChecker singular links", "[notifications]") { r->update_schema({{ "table", { - {"int", PropertyType::Int}, + {"int", PropertyType::Int, Property::IsPrimary{true}}, {"link1", PropertyType::Object | PropertyType::Nullable, "table"}, {"link2", PropertyType::Object | PropertyType::Nullable, "table"}, {"mixed_link", PropertyType::Mixed | PropertyType::Nullable}, @@ -1847,7 +1851,7 @@ TEST_CASE("DeepChangeChecker singular links", "[notifications]") { std::vector objects; r->begin_transaction(); for (int i = 0; i < 10; ++i) - objects.push_back(table->create_object().set_all(i)); + objects.push_back(table->create_object_with_primary_key(i)); r->commit_transaction(); auto track_changes = [&](auto&& f) { @@ -2124,7 +2128,7 @@ TEST_CASE("DeepChangeChecker singular links", "[notifications]") { table->clear(); objects.clear(); for (int i = 0; i < 20; ++i) - objects.push_back(table->create_object().set_all(i)); + objects.push_back(table->create_object_with_primary_key(i)); for (int i = 0; i < 19; ++i) set_link(objects[i], link_column, ObjLink{dst_table_key, objects[i + 1].get_key()}); r->commit_transaction(); diff --git a/test/test_global_key.cpp b/test/test_global_key.cpp deleted file mode 100644 index e504d53c2ab..00000000000 --- a/test/test_global_key.cpp +++ /dev/null @@ -1,77 +0,0 @@ -/************************************************************************* - * - * Copyright 2016 Realm Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - **************************************************************************/ - -#include - -#include "test.hpp" - -#include -#include - -using namespace realm; - -TEST(GlobalKey_ToString) -{ - CHECK_EQUAL(GlobalKey(0xabc, 0xdef).to_string(), "{0abc-0def}"); - CHECK_EQUAL(GlobalKey(0x11abc, 0x999def).to_string(), "{11abc-999def}"); - CHECK_EQUAL(GlobalKey(0, 0).to_string(), "{0000-0000}"); -} - -TEST(GlobalKey_FromString) -{ - CHECK_EQUAL(GlobalKey::from_string("{0-0}"), GlobalKey(0, 0)); - CHECK_EQUAL(GlobalKey::from_string("{aaaabbbbccccdddd-eeeeffff00001111}"), - GlobalKey(0xaaaabbbbccccddddULL, 0xeeeeffff00001111ULL)); - CHECK_THROW(GlobalKey::from_string(""), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{}"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("}"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("0"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{0}"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("-"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("0-"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{0-0"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{0-0-0}"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{aaaabbbbccccdddde-0}"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{0g-0}"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{0-0g}"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{0-aaaabbbbccccdddde}"), InvalidArgument); - CHECK_THROW(GlobalKey::from_string("{-}"), InvalidArgument); - - // std::strtoull accepts the "0x" prefix. We don't. - CHECK_THROW(GlobalKey::from_string("{0x0-0x0}"), InvalidArgument); - { - std::istringstream istr("{1-2}"); - GlobalKey oid; - istr >> oid; - CHECK_EQUAL(oid, GlobalKey(1, 2)); - } - { - std::istringstream istr("{1-2"); - GlobalKey oid; - istr >> oid; - CHECK(istr.rdstate() & std::istream::failbit); - CHECK_EQUAL(oid, GlobalKey()); - } -} - -TEST(GlobalKey_Compare) -{ - CHECK_LESS(GlobalKey(0, 0), GlobalKey(0, 1)); - CHECK_LESS(GlobalKey(0, 0), GlobalKey(1, 0)); -} diff --git a/test/test_mixed_null_assertions.cpp b/test/test_mixed_null_assertions.cpp index 33a5ea2984a..f5dc6db194e 100644 --- a/test/test_mixed_null_assertions.cpp +++ b/test/test_mixed_null_assertions.cpp @@ -89,10 +89,10 @@ TEST(List_Mixed_do_insert) TEST(Mixed_List_unresolved_as_null) { Group g; - auto t = g.add_table("foo"); + auto t = g.add_table_with_primary_key("foo", type_Int, "id"); t->add_column_list(type_Mixed, "mixeds"); - auto obj = t->create_object(); - auto obj1 = t->create_object(); + auto obj = t->create_object_with_primary_key(1); + auto obj1 = t->create_object_with_primary_key(2); auto list = obj.get_list("mixeds"); @@ -168,10 +168,10 @@ TEST(Mixed_List_unresolved_as_null) { Group g; - auto t = g.add_table("foo"); + auto t = g.add_table_with_primary_key("foo", type_Int, "id"); t->add_column_list(type_Mixed, "mixeds"); - auto obj = t->create_object(); - auto obj1 = t->create_object(); + auto obj = t->create_object_with_primary_key(1); + auto obj1 = t->create_object_with_primary_key(2); auto list = obj.get_list("mixeds"); list.insert(0, obj1); @@ -186,10 +186,10 @@ TEST(Mixed_List_unresolved_as_null) { Group g; - auto t = g.add_table("foo"); + auto t = g.add_table_with_primary_key("foo", type_Int, "id"); t->add_column_list(type_Mixed, "mixeds"); - auto obj = t->create_object(); - auto obj1 = t->create_object(); + auto obj = t->create_object_with_primary_key(1); + auto obj1 = t->create_object_with_primary_key(2); auto list = obj.get_list("mixeds"); list.insert(0, obj1); @@ -205,11 +205,11 @@ TEST(Mixed_Set_unresolved_links) { Group g; - auto t = g.add_table("foo"); + auto t = g.add_table_with_primary_key("foo", type_Int, "id"); t->add_column_set(type_Mixed, "mixeds"); - auto obj = t->create_object(); - auto obj1 = t->create_object(); - auto obj2 = t->create_object(); + auto obj = t->create_object_with_primary_key(1); + auto obj1 = t->create_object_with_primary_key(2); + auto obj2 = t->create_object_with_primary_key(3); auto set = obj.get_set("mixeds"); auto [it, success] = set.insert(Mixed{obj1}); obj1.invalidate(); @@ -261,11 +261,11 @@ TEST(Mixed_Set_unresolved_links) { // erase null but there are only unresolved links in the set Group g; - auto t = g.add_table("foo"); + auto t = g.add_table_with_primary_key("foo", type_Int, "id"); t->add_column_set(type_Mixed, "mixeds"); - auto obj = t->create_object(); - auto obj1 = t->create_object(); - auto obj2 = t->create_object(); + auto obj = t->create_object_with_primary_key(1); + auto obj1 = t->create_object_with_primary_key(2); + auto obj2 = t->create_object_with_primary_key(3); auto set = obj.get_set("mixeds"); set.insert(obj1); set.insert(obj2); @@ -286,11 +286,11 @@ TEST(Mixed_Set_unresolved_links) { // erase null when there are unresolved and nulls Group g; - auto t = g.add_table("foo"); + auto t = g.add_table_with_primary_key("foo", type_Int, "id"); t->add_column_set(type_Mixed, "mixeds"); - auto obj = t->create_object(); - auto obj1 = t->create_object(); - auto obj2 = t->create_object(); + auto obj = t->create_object_with_primary_key(1); + auto obj1 = t->create_object_with_primary_key(2); + auto obj2 = t->create_object_with_primary_key(3); auto set = obj.get_set("mixeds"); set.insert(obj1); set.insert(obj2); @@ -312,11 +312,11 @@ TEST(Mixed_Set_unresolved_links) { // assure that random access iterator does not return an unresolved link Group g; - auto t = g.add_table("foo"); + auto t = g.add_table_with_primary_key("foo", type_Int, "id"); t->add_column_set(type_Mixed, "mixeds"); - auto obj = t->create_object(); - auto obj1 = t->create_object(); - auto obj2 = t->create_object(); + auto obj = t->create_object_with_primary_key(1); + auto obj1 = t->create_object_with_primary_key(2); + auto obj2 = t->create_object_with_primary_key(3); auto set = obj.get_set("mixeds"); set.insert(obj1); set.insert(obj2); diff --git a/test/test_set.cpp b/test/test_set.cpp index 29375a83cff..24f6e81a876 100644 --- a/test/test_set.cpp +++ b/test/test_set.cpp @@ -241,7 +241,7 @@ TEST(Set_Links) { Group g; auto foos = g.add_table("class_Foo"); - auto bars = g.add_table("class_Bar"); + auto bars = g.add_table_with_primary_key("class_Bar", type_Int, "id"); auto cabs = g.add_table("class_Cab"); ColKey col_links = foos->add_column_set(*bars, "links"); @@ -249,10 +249,10 @@ TEST(Set_Links) auto foo = foos->create_object(); - auto bar1 = bars->create_object(); - auto bar2 = bars->create_object(); - auto bar3 = bars->create_object(); - auto bar4 = bars->create_object(); + auto bar1 = bars->create_object_with_primary_key(1); + auto bar2 = bars->create_object_with_primary_key(2); + auto bar3 = bars->create_object_with_primary_key(3); + auto bar4 = bars->create_object_with_primary_key(4); auto cab1 = cabs->create_object(); auto cab2 = cabs->create_object(); @@ -473,13 +473,13 @@ TEST(Set_LnkSetUnresolved) { Group g; auto foos = g.add_table("class_Foo"); - auto bars = g.add_table("class_Bar"); + auto bars = g.add_table_with_primary_key("class_Bar", type_Int, "id"); ColKey col_links = foos->add_column_set(*bars, "links"); auto foo = foos->create_object(); - auto bar1 = bars->create_object(); - auto bar2 = bars->create_object(); - auto bar3 = bars->create_object(); + auto bar1 = bars->create_object_with_primary_key(1); + auto bar2 = bars->create_object_with_primary_key(2); + auto bar3 = bars->create_object_with_primary_key(3); auto key_set = foo.get_set(col_links); auto link_set = foo.get_linkset(col_links); diff --git a/test/test_stable_ids.cpp b/test/test_stable_ids.cpp deleted file mode 100644 index e28a6cf7896..00000000000 --- a/test/test_stable_ids.cpp +++ /dev/null @@ -1,322 +0,0 @@ -#include "test.hpp" - -#include -#include -#include -#include - -#include -#include -#include - -using namespace realm; -using namespace realm::sync; - - -namespace { - -struct MakeClientHistory { - static std::unique_ptr make_history() - { - return realm::sync::make_client_replication(); - } -}; - -struct MakeServerHistory { - class HistoryContext : public _impl::ServerHistory::Context { - public: - std::mt19937_64& server_history_get_random() noexcept override final - { - return m_random; - } - - private: - std::mt19937_64 m_random; - }; - class WrapServerHistory : public HistoryContext, public _impl::ServerHistory { - public: - WrapServerHistory() - : _impl::ServerHistory{static_cast(*this)} - { - } - }; - - static std::unique_ptr<_impl::ServerHistory> make_history() - { - return std::make_unique(); - } -}; - -} // unnamed namespace - - -TEST_TYPES(InstructionReplication_CreateIdColumnInNewTables, MakeClientHistory, MakeServerHistory) -{ - SHARED_GROUP_TEST_PATH(test_dir); - auto history = TEST_TYPE::make_history(); - DBRef sg = DB::create(*history, test_dir); - - { - WriteTransaction wt{sg}; - wt.get_or_add_table("class_foo"); - wt.commit(); - } - - // Check that only the AddTable instruction is emitted - Changeset result; - auto buffer = history->get_instruction_encoder().release(); - util::SimpleInputStream stream{buffer}; - sync::parse_changeset(stream, result); - CHECK_EQUAL(result.size(), 1); - CHECK_EQUAL(result.begin()->type(), Instruction::Type::AddTable); - auto& instr = result.begin()->get_as(); - CHECK_EQUAL(result.get_string(instr.table), "foo"); - - auto rt = sg->start_read(); - ConstTableRef foo = rt->get_table("class_foo"); - CHECK(foo); - CHECK_EQUAL(foo->get_column_count(), 0); -} - -TEST_TYPES(InstructionReplication_PopulatesObjectIdColumn, MakeClientHistory, MakeServerHistory) -{ - SHARED_GROUP_TEST_PATH(test_dir); - auto history = TEST_TYPE::make_history(); - - DBRef sg = DB::create(*history, test_dir); - - auto client_file_ident = sg->start_read()->get_sync_file_id(); - - // Tables without primary keys: - { - WriteTransaction wt{sg}; - TableRef t0 = wt.get_or_add_table("class_t0"); - - auto obj0 = t0->create_object(); - auto obj1 = t0->create_object(); - - // Object IDs should be peerID plus a sequence number - CHECK_EQUAL(obj0.get_object_id(), GlobalKey(client_file_ident, 0)); - CHECK_EQUAL(obj1.get_object_id(), GlobalKey(client_file_ident, 1)); - } - - // Tables with integer primary keys: - { - WriteTransaction wt{sg}; - TableRef t1 = wt.get_group().add_table_with_primary_key("class_t1", type_Int, "pk"); - auto obj0 = t1->create_object_with_primary_key(123); - - GlobalKey expected_object_id(123); - CHECK_EQUAL(obj0.get_object_id(), expected_object_id); - } - - // Tables with string primary keys: - { - WriteTransaction wt{sg}; - TableRef t2 = wt.get_group().add_table_with_primary_key("class_t2", type_String, "pk"); - auto obj0 = t2->create_object_with_primary_key("foo"); - - GlobalKey expected_object_id("foo"); - CHECK_EQUAL(obj0.get_object_id(), expected_object_id); - } - - // Attempting to create a table that already exists is a no-op if the same primary key name, type and nullability - // is used. - { - WriteTransaction wt{sg}; - TableRef t1 = wt.get_group().get_or_add_table_with_primary_key("class_t1", type_Int, "pk"); - TableRef t11 = wt.get_group().get_or_add_table_with_primary_key("class_t1", type_Int, "pk"); - CHECK_EQUAL(t1, t11); - - TableRef t2 = - wt.get_group().get_or_add_table_with_primary_key("class_t2", type_Int, "pk", /* nullable */ true); - TableRef t21 = - wt.get_group().get_or_add_table_with_primary_key("class_t2", type_Int, "pk", /* nullable */ true); - CHECK_EQUAL(t2, t21); - - TableRef t3 = wt.get_group().get_or_add_table_with_primary_key("class_t3", type_String, "pk"); - TableRef t31 = wt.get_group().get_or_add_table_with_primary_key("class_t3", type_String, "pk"); - CHECK_EQUAL(t3, t31); - - TableRef t4 = - wt.get_group().get_or_add_table_with_primary_key("class_t4", type_String, "pk", /* nullable */ true); - TableRef t41 = - wt.get_group().get_or_add_table_with_primary_key("class_t4", type_String, "pk", /* nullable */ true); - CHECK_EQUAL(t4, t41); - } - - // Attempting to create a table that already exists causes an assertion failure if different primary key name, - // type, or nullability is specified. This is not currently testable. -} - -TEST(StableIDs_ChangesGlobalObjectIdWhenPeerIdReceived) -{ - SHARED_GROUP_TEST_PATH(test_dir); - auto repl = make_client_replication(); - - DBRef sg = DB::create(*repl, test_dir); - - ColKey link_col; - { - WriteTransaction wt{sg}; - TableRef t0 = wt.get_or_add_table("class_t0"); - TableRef t1 = wt.get_or_add_table("class_t1"); - link_col = t0->add_column(*t1, "link"); - - Obj t1_k1 = t1->create_object(); - Obj t0_k1 = t0->create_object().set(link_col, t1_k1.get_key()); - Obj t0_k2 = t0->create_object(); - - // Object IDs should be peerID plus a sequence number - CHECK_EQUAL(t0_k1.get_object_id(), GlobalKey(0, 0)); - CHECK_EQUAL(t0_k2.get_object_id(), GlobalKey(0, 1)); - wt.commit(); - } - - bool fix_up_object_ids = true; - auto& history = repl->get_history(); - history.set_client_file_ident({1, 123}, fix_up_object_ids); - - // Save the changeset to replay later - UploadCursor upload_cursor{0, 0}; - std::vector changesets; - version_type locked_server_version; // Dummy - history.find_uploadable_changesets(upload_cursor, 2, changesets, locked_server_version); - CHECK_GREATER_EQUAL(changesets.size(), 1); - auto& changeset = changesets[0].changeset; - ChunkedBinaryInputStream stream{changeset}; - Changeset result; - sync::parse_changeset(stream, result); - - // Check that ObjectIds gets translated correctly - { - ReadTransaction rt{sg}; - ConstTableRef t0 = rt.get_table("class_t0"); - ConstTableRef t1 = rt.get_table("class_t1"); - auto it = t0->begin(); - GlobalKey oid0 = it->get_object_id(); - ObjKey link_ndx = it->get(link_col); - ++it; - GlobalKey oid1 = it->get_object_id(); - CHECK_EQUAL(oid0, GlobalKey(1, 0)); - CHECK_EQUAL(oid1, GlobalKey(1, 1)); - GlobalKey oid2 = t1->get_object_id(link_ndx); - CHECK_EQUAL(oid2.hi(), 1); - CHECK_EQUAL(oid2, t1->begin()->get_object_id()); - } - - // Replay the transaction to see that the instructions were modified. - { - SHARED_GROUP_TEST_PATH(test_dir_2); - auto history_2 = make_client_replication(); - DBRef sg_2 = DB::create(*history_2, test_dir_2); - - WriteTransaction wt{sg_2}; - InstructionApplier applier{wt}; - applier.apply(result); - wt.commit(); - - // Check same invariants as above. - ReadTransaction rt{sg_2}; - ConstTableRef t0 = rt.get_table("class_t0"); - ConstTableRef t1 = rt.get_table("class_t1"); - auto it = t0->begin(); - GlobalKey oid0 = it->get_object_id(); - ObjKey link_ndx = it->get(link_col); - ++it; - GlobalKey oid1 = it->get_object_id(); - CHECK_EQUAL(oid0, GlobalKey(1, 0)); - CHECK_EQUAL(oid1, GlobalKey(1, 1)); - GlobalKey oid2 = t1->get_object_id(link_ndx); - CHECK_EQUAL(oid2.hi(), 1); - CHECK_EQUAL(oid2, t1->begin()->get_object_id()); - } -} - -TEST_TYPES(StableIDs_PersistPerTableSequenceNumber, MakeClientHistory, MakeServerHistory) -{ - SHARED_GROUP_TEST_PATH(test_dir); - { - auto history = TEST_TYPE::make_history(); - DBRef sg = DB::create(*history, test_dir); - WriteTransaction wt{sg}; - TableRef t0 = wt.get_or_add_table("class_t0"); - t0->create_object(); - t0->create_object(); - CHECK_EQUAL(t0->size(), 2); - wt.commit(); - } - { - auto history = TEST_TYPE::make_history(); - DBRef sg = DB::create(*history, test_dir); - WriteTransaction wt{sg}; - TableRef t0 = wt.get_or_add_table("class_t0"); - t0->create_object(); - t0->create_object(); - CHECK_EQUAL(t0->size(), 4); - wt.commit(); - } -} - -TEST_TYPES(StableIDs_CollisionMapping, MakeClientHistory, MakeServerHistory) -{ -#if REALM_EXERCISE_OBJECT_ID_COLLISION - - // This number corresponds to the mask used to calculate "optimistic" - // object IDs. See `GlobalKeyProvider::get_optimistic_local_id_hashed`. - const size_t num_objects_with_guaranteed_collision = 0xff; - - SHARED_GROUP_TEST_PATH(test_dir); - - { - auto history = TEST_TYPE::make_history(); - DBRef sg = DB::create(*history, test_dir); - { - WriteTransaction wt{sg}; - TableRef t0 = wt.get_group().add_table_with_primary_key("class_t0", type_String, "pk"); - - char buffer[12]; - for (size_t i = 0; i < num_objects_with_guaranteed_collision; ++i) { - const char* in = reinterpret_cast(&i); - size_t len = base64_encode(in, sizeof(i), buffer, sizeof(buffer)); - - sync::create_object_with_primary_key(wt, *t0, StringData{buffer, len}); - } - wt.commit(); - } - - { - ReadTransaction rt{sg}; - ConstTableRef t0 = rt.get_table("class_t0"); - // Check that at least one object exists where the 63rd bit is set. - size_t num_object_keys_with_63rd_bit_set = 0; - uint64_t bit63 = 0x4000000000000000; - for (Obj obj : *t0) { - if (obj.get_key().value & bit63) - ++num_object_keys_with_63rd_bit_set; - } - CHECK_GREATER(num_object_keys_with_63rd_bit_set, 0); - } - } - - // Check that locally allocated IDs are properly persisted - { - auto history_2 = TEST_TYPE::make_history(); - DBRef sg_2 = DB::create(*history_2, test_dir); - WriteTransaction wt{sg_2}; - TableRef t0 = wt.get_table("class_t0"); - - // Make objects with primary keys that do not already exist but are guaranteed - // to cause further collisions. - char buffer[12]; - for (size_t i = 0; i < num_objects_with_guaranteed_collision; ++i) { - size_t foo = num_objects_with_guaranteed_collision + i; - const char* in = reinterpret_cast(&foo); - size_t len = base64_encode(in, sizeof(foo), buffer, sizeof(buffer)); - - sync::create_object_with_primary_key(wt, *t0, StringData{buffer, len}); - } - } - -#endif // REALM_EXERCISE_ID_COLLISION -} diff --git a/test/test_sync.cpp b/test/test_sync.cpp index b541c3ca2b9..4bf32453b8f 100644 --- a/test/test_sync.cpp +++ b/test/test_sync.cpp @@ -5548,49 +5548,6 @@ TEST(Sync_ServerSideEncryption) } -// This test calls row_for_object_id() for various object ids and tests that -// the right value is returned including that no assertions are hit. -TEST(Sync_RowForGlobalKey) -{ - TEST_CLIENT_DB(db); - - { - WriteTransaction wt(db); - TableRef table = wt.add_table("class_foo"); - table->add_column(type_Int, "i"); - wt.commit(); - } - - // Check that various object_ids are not in the table. - { - ReadTransaction rt(db); - ConstTableRef table = rt.get_table("class_foo"); - CHECK(table); - - // Default constructed GlobalKey - { - GlobalKey object_id; - auto row_ndx = table->get_objkey(object_id); - CHECK_NOT(row_ndx); - } - - // GlobalKey with small lo and hi values - { - GlobalKey object_id{12, 24}; - auto row_ndx = table->get_objkey(object_id); - CHECK_NOT(row_ndx); - } - - // GlobalKey with lo and hi values past the 32 bit limit. - { - GlobalKey object_id{uint_fast64_t(1) << 50, uint_fast64_t(1) << 52}; - auto row_ndx = table->get_objkey(object_id); - CHECK_NOT(row_ndx); - } - } -} - - TEST(Sync_LogCompaction_EraseObject_LinkList) { TEST_DIR(dir); diff --git a/test/test_unresolved_links.cpp b/test/test_unresolved_links.cpp index db675150829..a59d86e55bb 100644 --- a/test/test_unresolved_links.cpp +++ b/test/test_unresolved_links.cpp @@ -53,7 +53,7 @@ TEST(Unresolved_Basic) col_owns = persons->add_column(*cars, "car"); auto dealers = wt->add_table_with_primary_key("Dealer", type_Int, "cvr"); col_has = dealers->add_column_list(*cars, "stock"); - auto parts = wt->add_table("Parts"); // No primary key + auto parts = wt->add_table_with_primary_key("Parts", type_String, "id"); // No primary key col_part = cars->add_column(*parts, "part"); auto finn = persons->create_object_with_primary_key("finn.schiermer-andersen@mongodb.com"); @@ -65,7 +65,7 @@ TEST(Unresolved_Basic) auto stock = joergen.get_list(col_has); auto skoda = cars->create_object_with_primary_key("Skoda Fabia").set(col_price, Decimal128("149999.5")); - auto thingamajig = parts->create_object(); + auto thingamajig = parts->create_object_with_primary_key("abc-123"); skoda.set(col_part, thingamajig.get_key()); auto new_tesla = cars->get_objkey_from_primary_key("Tesla 10"); @@ -78,7 +78,7 @@ TEST(Unresolved_Basic) stock.insert(1, skoda.get_key()); // Create a tombstone implicitly - auto doodad = parts->get_objkey_from_global_key(GlobalKey{999, 999}); + auto doodad = parts->get_objkey_from_primary_key("def-123"); CHECK(doodad.is_unresolved()); CHECK_EQUAL(parts->nb_unresolved(), 1); @@ -138,12 +138,11 @@ TEST(Unresolved_Basic) auto parts = wt->get_table("Parts"); auto tesla = wt->get_table("Car")->create_object_with_primary_key("Tesla 10"); tesla.set(col_price, Decimal128("499999.5")); - auto doodad = parts->create_object(GlobalKey{999, 999}); - auto doodad1 = parts->create_object(GlobalKey{999, 999}); // Check idempotency + auto doodad = parts->create_object_with_primary_key("def-123"); + auto doodad1 = parts->create_object_with_primary_key("def-123"); // Check idempotency CHECK_EQUAL(doodad.get_key(), doodad1.get_key()); - CHECK_EQUAL(doodad.get_object_id(), doodad1.get_object_id()); tesla.set(col_part, doodad.get_key()); - auto doodad_key = parts->get_objkey_from_global_key(GlobalKey{999, 999}); + auto doodad_key = parts->get_objkey_from_primary_key("def-123"); CHECK(!doodad_key.is_unresolved()); CHECK_EQUAL(wt->get_table("Parts")->nb_unresolved(), 0); @@ -165,13 +164,13 @@ TEST(Unresolved_InvalidateObject) auto cars = g.add_table_with_primary_key("Car", type_String, "model"); auto col_wheels = cars->add_column_list(*wheels, "wheels"); auto col_price = cars->add_column(type_Decimal, "price"); - auto dealers = g.add_table("Dealer"); + auto dealers = g.add_table_with_primary_key("Dealer", type_Int, "id"); auto col_has = dealers->add_column_list(*cars, "stock"); auto organization = g.add_table("Organization"); auto col_members = organization->add_column_list(*dealers, "members"); - auto dealer1 = dealers->create_object(); - auto dealer2 = dealers->create_object(); + auto dealer1 = dealers->create_object_with_primary_key(1); + auto dealer2 = dealers->create_object_with_primary_key(2); auto org = organization->create_object(); auto members = org.get_linklist(col_members); diff --git a/test/util/compare_groups.cpp b/test/util/compare_groups.cpp index 67f7957bc3f..a50f32cc1ba 100644 --- a/test/util/compare_groups.cpp +++ b/test/util/compare_groups.cpp @@ -196,8 +196,8 @@ sync::PrimaryKey primary_key_for_row(const Obj& obj) REALM_TERMINATE("Missing primary key type support"); } - GlobalKey global_key = obj.get_object_id(); - return global_key; + REALM_TERMINATE("Missing primary key"); + return {}; } sync::PrimaryKey primary_key_for_row(const Table& table, ObjKey key) @@ -259,12 +259,7 @@ ObjKey row_for_primary_key(const Table& table, sync::PrimaryKey key) REALM_TERMINATE("row_for_primary_key missing primary key type support"); } - if (auto global_key = mpark::get_if(&key)) { - return table.get_objkey(*global_key); - } - else { - REALM_TERMINATE("row_for_primary_key() with primary key, expected GlobalKey"); - } + REALM_TERMINATE("row_for_primary_key() expected primary key"); return {}; } From 3016f20e52b09204b3d8c0e950b10d2d07268177 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Tue, 10 Oct 2023 12:01:20 +0200 Subject: [PATCH 2/2] Remove ClientHistory::fix_up_client_file_ident_in_stored_changesets() and friends --- src/realm/sync/client_base.hpp | 8 -- src/realm/sync/noinst/client_history_impl.cpp | 98 +------------------ src/realm/sync/noinst/client_history_impl.hpp | 8 +- src/realm/sync/noinst/client_impl_base.cpp | 4 +- src/realm/sync/noinst/client_impl_base.hpp | 4 - .../sync/tools/apply_to_state_command.cpp | 2 +- test/sync_fixtures.hpp | 1 - test/test_instruction_replication.cpp | 4 +- test/test_sync.cpp | 10 +- 9 files changed, 10 insertions(+), 129 deletions(-) diff --git a/src/realm/sync/client_base.hpp b/src/realm/sync/client_base.hpp index 71b9722ea18..92302edac82 100644 --- a/src/realm/sync/client_base.hpp +++ b/src/realm/sync/client_base.hpp @@ -209,14 +209,6 @@ struct ClientConfig { /// /// Testing/debugging feature. Should never be enabled in production. bool disable_sync_to_disk = false; - - /// The sync client supports tables without primary keys by synthesizing a - /// pk using the client file ident, which means that all changesets waiting - /// to be uploaded need to be rewritten with the correct ident the first time - /// we connect to the server. The modern server doesn't support this and - /// requires pks for all tables, so this is now only applicable to old sync - /// tests and so is disabled by default. - bool fix_up_object_ids = false; }; /// \brief Information about an error causing a session to be temporarily diff --git a/src/realm/sync/noinst/client_history_impl.cpp b/src/realm/sync/noinst/client_history_impl.cpp index 527f3f3bf2b..775b6a2642c 100644 --- a/src/realm/sync/noinst/client_history_impl.cpp +++ b/src/realm/sync/noinst/client_history_impl.cpp @@ -270,7 +270,7 @@ void ClientHistory::get_status(version_type& current_client_version, SaltedFileI } -void ClientHistory::set_client_file_ident(SaltedFileIdent client_file_ident, bool fix_up_object_ids) +void ClientHistory::set_client_file_ident(SaltedFileIdent client_file_ident) { REALM_ASSERT(client_file_ident.ident != 0); @@ -287,10 +287,6 @@ void ClientHistory::set_client_file_ident(SaltedFileIdent client_file_ident, boo root.set(s_progress_download_client_version_iip, RefOrTagged::make_tagged(0)); root.set(s_progress_upload_client_version_iip, RefOrTagged::make_tagged(0)); - if (fix_up_object_ids) { - fix_up_client_file_ident_in_stored_changesets(*wt, client_file_ident.ident); // Throws - } - // Note: This transaction produces an empty changeset. Empty changesets are // not uploaded to the server. wt->commit(); // Throws @@ -1024,98 +1020,6 @@ void ClientHistory::do_trim_sync_history(std::size_t n) } } -void ClientHistory::fix_up_client_file_ident_in_stored_changesets(Transaction& group, - file_ident_type client_file_ident) -{ - // Must be in write transaction! - - REALM_ASSERT(client_file_ident != 0); - auto promote_global_key = [client_file_ident](GlobalKey* oid) { - if (oid->hi() == 0) { - // client_file_ident == 0 - *oid = GlobalKey{uint64_t(client_file_ident), oid->lo()}; - return true; - } - return false; - }; - - Group::TableNameBuffer buffer; - auto get_table_for_class = [&](StringData class_name) -> ConstTableRef { - REALM_ASSERT(class_name.size() < Group::max_table_name_length - 6); - return group.get_table(Group::class_name_to_table_name(class_name, buffer)); - }; - - util::compression::CompressMemoryArena arena; - util::AppendBuffer compressed; - - // Fix up changesets. - Array& root = m_arrays->root; - uint64_t uploadable_bytes = root.get_as_ref_or_tagged(s_progress_uploadable_bytes_iip).get_as_int(); - for (size_t i = 0; i < sync_history_size(); ++i) { - // We could have opened a pre-provisioned realm file. In this case we can skip the entries downloaded - // from the server. - if (m_arrays->origin_file_idents.get(i) != 0) - continue; - - ChunkedBinaryData changeset{m_arrays->changesets, i}; - ChunkedBinaryInputStream is{changeset}; - size_t decompressed_size; - auto decompressed = util::compression::decompress_nonportable_input_stream(is, decompressed_size); - if (!decompressed) - continue; - Changeset log; - parse_changeset(*decompressed, log); - - bool did_modify = false; - auto last_class_name = InternString::npos; - ConstTableRef selected_table; - for (auto instr : log) { - if (!instr) - continue; - - if (auto obj_instr = instr->get_if()) { - // Cache the TableRef - if (obj_instr->table != last_class_name) { - StringData class_name = log.get_string(obj_instr->table); - last_class_name = obj_instr->table; - selected_table = get_table_for_class(class_name); - } - - // Fix up instructions using GlobalKey to identify objects. - if (auto global_key = mpark::get_if(&obj_instr->object)) { - did_modify = promote_global_key(global_key); - } - - // Fix up the payload for Set and ArrayInsert. - Instruction::Payload* payload = nullptr; - if (auto set_instr = instr->get_if()) { - payload = &set_instr->value; - } - else if (auto list_insert_instr = instr->get_if()) { - payload = &list_insert_instr->value; - } - - if (payload && payload->type == Instruction::Payload::Type::Link) { - if (auto global_key = mpark::get_if(&payload->data.link.target)) { - did_modify = promote_global_key(global_key); - } - } - } - } - - if (did_modify) { - ChangesetEncoder::Buffer modified; - encode_changeset(log, modified); - util::compression::allocate_and_compress_nonportable(arena, modified, compressed); - m_arrays->changesets.set(i, BinaryData{compressed.data(), compressed.size()}); // Throws - - uploadable_bytes += modified.size() - decompressed_size; - } - } - - root.set(s_progress_uploadable_bytes_iip, RefOrTagged::make_tagged(uploadable_bytes)); -} - void ClientHistory::set_group(Group* group, bool updated) { _impl::History::set_group(group, updated); diff --git a/src/realm/sync/noinst/client_history_impl.hpp b/src/realm/sync/noinst/client_history_impl.hpp index 42244b69f0f..80d7a69ab1d 100644 --- a/src/realm/sync/noinst/client_history_impl.hpp +++ b/src/realm/sync/noinst/client_history_impl.hpp @@ -151,16 +151,11 @@ class ClientHistory final : public _impl::History, public TransformHistory { /// identical identifiers for two client files if they are associated with /// different server Realms. /// - /// \param fix_up_object_ids The object ids that depend on client file ident - /// will be fixed in both state and history if this parameter is true. If - /// it is known that there are no objects to fix, it can be set to false to - /// achieve higher performance. - /// /// The client is required to obtain the file identifier before engaging in /// synchronization proper, and it must store the identifier and use it to /// reestablish the connection between the client file and the server file /// when engaging in future synchronization sessions. - void set_client_file_ident(SaltedFileIdent client_file_ident, bool fix_up_object_ids); + void set_client_file_ident(SaltedFileIdent client_file_ident); /// Stores the synchronization progress in the associated Realm file in a /// way that makes it available via get_status() during future @@ -428,7 +423,6 @@ class ClientHistory final : public _impl::History, public TransformHistory { void do_trim_sync_history(std::size_t n); void clamp_sync_version_range(version_type& begin, version_type& end) const noexcept; Transformer& get_transformer(); - void fix_up_client_file_ident_in_stored_changesets(Transaction&, file_ident_type); void record_current_schema_version(); static void record_current_schema_version(Array& schema_versions, version_type snapshot_version); void compress_stored_changesets(); diff --git a/src/realm/sync/noinst/client_impl_base.cpp b/src/realm/sync/noinst/client_impl_base.cpp index 9725a0aa10b..1dd81b53ca2 100644 --- a/src/realm/sync/noinst/client_impl_base.cpp +++ b/src/realm/sync/noinst/client_impl_base.cpp @@ -149,7 +149,6 @@ ClientImpl::ClientImpl(ClientConfig config) , m_dry_run{config.dry_run} , m_enable_default_port_hack{config.enable_default_port_hack} , m_disable_upload_compaction{config.disable_upload_compaction} - , m_fix_up_object_ids{config.fix_up_object_ids} , m_roundtrip_time_handler{std::move(config.roundtrip_time_handler)} , m_socket_provider{std::move(config.socket_provider)} , m_client_protocol{} // Throws @@ -2333,8 +2332,7 @@ Status Session::receive_ident_message(SaltedFileIdent client_file_ident) return Status::OK(); } if (!did_client_reset) { - repl.get_history().set_client_file_ident(client_file_ident, - m_fix_up_object_ids); // Throws + repl.get_history().set_client_file_ident(client_file_ident); // Throws m_progress.download.last_integrated_client_version = 0; m_progress.upload.client_version = 0; m_last_version_selected_for_upload = 0; diff --git a/src/realm/sync/noinst/client_impl_base.hpp b/src/realm/sync/noinst/client_impl_base.hpp index a95e1e65708..4065fc4afce 100644 --- a/src/realm/sync/noinst/client_impl_base.hpp +++ b/src/realm/sync/noinst/client_impl_base.hpp @@ -250,7 +250,6 @@ class ClientImpl { const bool m_dry_run; // For testing purposes only const bool m_enable_default_port_hack; const bool m_disable_upload_compaction; - const bool m_fix_up_object_ids; const std::function m_roundtrip_time_handler; const std::string m_user_agent_string; std::shared_ptr m_socket_provider; @@ -1050,8 +1049,6 @@ class ClientImpl::Session { bool m_is_flx_sync_session = false; - bool m_fix_up_object_ids = false; - // These are reset when the session is activated, and again whenever the // connection is lost or the rebinding process is initiated. bool m_enlisted_to_send; @@ -1464,7 +1461,6 @@ inline ClientImpl::Session::Session(SessionWrapper& wrapper, Connection& conn, s , m_ident{ident} , m_try_again_delay_info(conn.get_client().m_reconnect_backoff_info, conn.get_client().get_random()) , m_is_flx_sync_session(conn.is_flx_sync_connection()) - , m_fix_up_object_ids(get_client().m_fix_up_object_ids) , m_wrapper{wrapper} { if (get_client().m_disable_upload_activation_delay) diff --git a/src/realm/sync/tools/apply_to_state_command.cpp b/src/realm/sync/tools/apply_to_state_command.cpp index 3a6aa01498d..8fa315da42d 100644 --- a/src/realm/sync/tools/apply_to_state_command.cpp +++ b/src/realm/sync/tools/apply_to_state_command.cpp @@ -314,7 +314,7 @@ int main(int argc, const char** argv) } }, [&](const ServerIdentMessage& ident_message) { - history.set_client_file_ident(ident_message.file_ident, true); + history.set_client_file_ident(ident_message.file_ident); }}, message); } diff --git a/test/sync_fixtures.hpp b/test/sync_fixtures.hpp index 720e15d608b..0c8ea3cb084 100644 --- a/test/sync_fixtures.hpp +++ b/test/sync_fixtures.hpp @@ -550,7 +550,6 @@ class MultiClientServerFixture { config_2.disable_upload_compaction = config.disable_upload_compaction; config_2.one_connection_per_session = config.one_connection_per_session; config_2.disable_upload_activation_delay = config.disable_upload_activation_delay; - config_2.fix_up_object_ids = true; m_clients[i] = std::make_unique(std::move(config_2)); } diff --git a/test/test_instruction_replication.cpp b/test/test_instruction_replication.cpp index 3c1c7239916..3b2e5dd3af8 100644 --- a/test/test_instruction_replication.cpp +++ b/test/test_instruction_replication.cpp @@ -31,9 +31,7 @@ struct Fixture { , sg_1(DB::create(*history_1, path_1)) , sg_2(DB::create(*history_2, path_2)) { - // This is to ensure that peer IDs in Object IDs are populated. - bool fix_up_object_ids = true; - history_1->get_history().set_client_file_ident({1, 123}, fix_up_object_ids); + history_1->get_history().set_client_file_ident({1, 123}); } void replay_transactions() diff --git a/test/test_sync.cpp b/test/test_sync.cpp index 4bf32453b8f..e5e09087ec7 100644 --- a/test/test_sync.cpp +++ b/test/test_sync.cpp @@ -6578,7 +6578,7 @@ TEST(Sync_DanglingLinksCountInPriorSize) ClientReplication repl; auto local_db = realm::DB::create(repl, path); auto& history = repl.get_history(); - history.set_client_file_ident(sync::SaltedFileIdent{1, 123456}, true); + history.set_client_file_ident(sync::SaltedFileIdent{1, 123456}); version_type last_version, last_version_observed = 0; auto dump_uploadable = [&] { @@ -6853,7 +6853,7 @@ TEST(Sync_NonIncreasingServerVersions) TEST_CLIENT_DB(db); auto& history = get_history(db); - history.set_client_file_ident(SaltedFileIdent{2, 0x1234567812345678}, false); + history.set_client_file_ident(SaltedFileIdent{2, 0x1234567812345678}); timestamp_type timestamp{1}; history.set_local_origin_timestamp_source([&] { return ++timestamp; @@ -6926,7 +6926,7 @@ TEST(Sync_InvalidChangesetFromServer) TEST_CLIENT_DB(db); auto& history = get_history(db); - history.set_client_file_ident(SaltedFileIdent{2, 0x1234567812345678}, false); + history.set_client_file_ident(SaltedFileIdent{2, 0x1234567812345678}); instr::CreateObject bad_instr; bad_instr.object = InternString{1}; @@ -7004,7 +7004,7 @@ TEST(Sync_SetAndGetEmptyReciprocalChangeset) TEST_CLIENT_DB(db); auto& history = get_history(db); - history.set_client_file_ident(SaltedFileIdent{1, 0x1234567812345678}, false); + history.set_client_file_ident(SaltedFileIdent{1, 0x1234567812345678}); timestamp_type timestamp{1}; history.set_local_origin_timestamp_source([&] { return ++timestamp; @@ -7173,7 +7173,7 @@ TEST(Sync_ServerVersionsSkippedFromDownloadCursor) TEST_CLIENT_DB(db); auto& history = get_history(db); - history.set_client_file_ident(SaltedFileIdent{2, 0x1234567812345678}, false); + history.set_client_file_ident(SaltedFileIdent{2, 0x1234567812345678}); timestamp_type timestamp{1}; history.set_local_origin_timestamp_source([&] { return ++timestamp;