From 6dfeaf8080e396f937f16f51a1efba425ececb68 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Tue, 15 Sep 2015 16:25:16 +0200 Subject: [PATCH 1/3] Move things which are not part of the API to an impl directory/namespace --- {apple => impl/apple}/external_commit_helper.cpp | 1 + {apple => impl/apple}/external_commit_helper.hpp | 2 ++ transact_log_handler.cpp => impl/transact_log_handler.cpp | 2 ++ transact_log_handler.hpp => impl/transact_log_handler.hpp | 2 ++ shared_realm.cpp | 1 + shared_realm.hpp | 7 +++++-- 6 files changed, 13 insertions(+), 2 deletions(-) rename {apple => impl/apple}/external_commit_helper.cpp (99%) rename {apple => impl/apple}/external_commit_helper.hpp (98%) rename transact_log_handler.cpp => impl/transact_log_handler.cpp (99%) rename transact_log_handler.hpp => impl/transact_log_handler.hpp (97%) diff --git a/apple/external_commit_helper.cpp b/impl/apple/external_commit_helper.cpp similarity index 99% rename from apple/external_commit_helper.cpp rename to impl/apple/external_commit_helper.cpp index 1444281a..4cf0b523 100644 --- a/apple/external_commit_helper.cpp +++ b/impl/apple/external_commit_helper.cpp @@ -30,6 +30,7 @@ #include using namespace realm; +using namespace realm::_impl; namespace { // Write a byte to a pipe to notify anyone waiting for data on the pipe diff --git a/apple/external_commit_helper.hpp b/impl/apple/external_commit_helper.hpp similarity index 98% rename from apple/external_commit_helper.hpp rename to impl/apple/external_commit_helper.hpp index acf0e1c4..d7acb791 100644 --- a/apple/external_commit_helper.hpp +++ b/impl/apple/external_commit_helper.hpp @@ -26,6 +26,7 @@ namespace realm { class Realm; +namespace _impl { class ExternalCommitHelper { public: ExternalCommitHelper(Realm* realm); @@ -87,6 +88,7 @@ private: FdHolder m_shutdown_write_fd; }; +} // namespace _impl } // namespace realm #endif /* REALM_EXTERNAL_COMMIT_HELPER_HPP */ diff --git a/transact_log_handler.cpp b/impl/transact_log_handler.cpp similarity index 99% rename from transact_log_handler.cpp rename to impl/transact_log_handler.cpp index 3f833b19..e1b95405 100644 --- a/transact_log_handler.cpp +++ b/impl/transact_log_handler.cpp @@ -316,6 +316,7 @@ public: } // anonymous namespace namespace realm { +namespace _impl { namespace transaction { void advance(SharedGroup& sg, ClientHistory& history, RealmDelegate* delegate) { TransactLogHandler(delegate, sg, [&](auto&&... args) { @@ -344,4 +345,5 @@ void cancel(SharedGroup& sg, ClientHistory& history, RealmDelegate* delegate) { } } // namespace transaction +} // namespace _impl } // namespace realm diff --git a/transact_log_handler.hpp b/impl/transact_log_handler.hpp similarity index 97% rename from transact_log_handler.hpp rename to impl/transact_log_handler.hpp index 3a77848f..dbb54a53 100644 --- a/transact_log_handler.hpp +++ b/impl/transact_log_handler.hpp @@ -24,6 +24,7 @@ class RealmDelegate; class SharedGroup; class ClientHistory; +namespace _impl { namespace transaction { // Advance the read transaction version, with change notifications sent to delegate // Must not be called from within a write transaction. @@ -41,6 +42,7 @@ void commit(SharedGroup& sg, ClientHistory& history, RealmDelegate* delegate); // for reverting to the old values sent to delegate void cancel(SharedGroup& sg, ClientHistory& history, RealmDelegate* delegate); } // namespace transaction +} // namespace _impl } // namespace realm #endif /* REALM_TRANSACT_LOG_HANDLER_HPP */ diff --git a/shared_realm.cpp b/shared_realm.cpp index 8b003d30..2ca84649 100644 --- a/shared_realm.cpp +++ b/shared_realm.cpp @@ -29,6 +29,7 @@ #include using namespace realm; +using namespace realm::_impl; RealmCache Realm::s_global_cache; diff --git a/shared_realm.hpp b/shared_realm.hpp index aa19071e..dc5c4b92 100644 --- a/shared_realm.hpp +++ b/shared_realm.hpp @@ -28,13 +28,16 @@ namespace realm { class ClientHistory; - class ExternalCommitHelper; class Realm; class RealmCache; class RealmDelegate; typedef std::shared_ptr SharedRealm; typedef std::weak_ptr WeakRealm; + namespace _impl { + class ExternalCommitHelper; + } + class Realm : public std::enable_shared_from_this { public: @@ -114,7 +117,7 @@ namespace realm { Group *m_group = nullptr; - std::shared_ptr m_notifier; + std::shared_ptr<_impl::ExternalCommitHelper> m_notifier; public: std::unique_ptr m_delegate; From 9f1702a10f2f6ec5e645dd2b7f325e410fd89b30 Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Mon, 19 Oct 2015 18:52:56 -0700 Subject: [PATCH 2/3] Support migrating required columns to optional, preserving their contents. Required columns are migrated to optional by creating a new nullable column, copying the data from the required column to the optional column, then removing the original required column. --- object_store.cpp | 66 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/object_store.cpp b/object_store.cpp index 035ce4fc..757d207f 100644 --- a/object_store.cpp +++ b/object_store.cpp @@ -149,6 +149,13 @@ static inline bool property_has_changed(Property const& p1, Property const& p2) || p1.is_nullable != p2.is_nullable; } +static inline bool property_can_be_migrated_to_nullable(const Property& old_property, const Property& new_property) { + return old_property.type == new_property.type + && !old_property.is_nullable + && new_property.is_nullable + && new_property.name == old_property.name; +} + void ObjectStore::verify_schema(Schema const& actual_schema, Schema& target_schema, bool allow_missing_tables) { std::vector errors; for (auto &object_schema : target_schema) { @@ -205,6 +212,45 @@ std::vector ObjectStore::verify_object_schema(O return exceptions; } +template +static void copy_property_values(const Property& old_property, const Property& new_property, Table& table, + T (Table::*getter)(std::size_t, std::size_t) const noexcept, + void (Table::*setter)(std::size_t, std::size_t, T)) { + size_t old_column = old_property.table_column, new_column = new_property.table_column; + size_t count = table.size(); + for (size_t i = 0; i < count; i++) { + (table.*setter)(new_column, i, (table.*getter)(old_column, i)); + } +} + +static void copy_property_values(const Property& source, const Property& destination, Table& table) { + switch (destination.type) { + case PropertyTypeInt: + copy_property_values(source, destination, table, &Table::get_int, &Table::set_int); + break; + case PropertyTypeBool: + copy_property_values(source, destination, table, &Table::get_bool, &Table::set_bool); + break; + case PropertyTypeFloat: + copy_property_values(source, destination, table, &Table::get_float, &Table::set_float); + break; + case PropertyTypeDouble: + copy_property_values(source, destination, table, &Table::get_double, &Table::set_double); + break; + case PropertyTypeString: + copy_property_values(source, destination, table, &Table::get_string, &Table::set_string); + break; + case PropertyTypeData: + copy_property_values(source, destination, table, &Table::get_binary, &Table::set_binary); + break; + case PropertyTypeDate: + copy_property_values(source, destination, table, &Table::get_datetime, &Table::set_datetime); + break; + default: + break; + } +} + // set references to tables on targetSchema and create/update any missing or out-of-date tables // if update existing is true, updates existing tables, otherwise validates existing tables // NOTE: must be called from within write transaction @@ -230,13 +276,31 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update ObjectSchema current_schema(group, target_object_schema->name); std::vector &target_props = target_object_schema->properties; + // handle columns changing from required to optional + for (auto& current_prop : current_schema.properties) { + auto target_prop = target_object_schema->property_for_name(current_prop.name); + if (!target_prop || !property_can_be_migrated_to_nullable(current_prop, *target_prop)) + continue; + + target_prop->table_column = current_prop.table_column; + current_prop.table_column = current_prop.table_column + 1; + + table->insert_column(target_prop->table_column, DataType(target_prop->type), target_prop->name, target_prop->is_nullable); + copy_property_values(current_prop, *target_prop, *table); + table->remove_column(current_prop.table_column); + + current_prop.table_column = target_prop->table_column; + changed = true; + } + // remove extra columns size_t deleted = 0; for (auto& current_prop : current_schema.properties) { current_prop.table_column -= deleted; auto target_prop = target_object_schema->property_for_name(current_prop.name); - if (!target_prop || property_has_changed(current_prop, *target_prop)) { + if (!target_prop || (property_has_changed(current_prop, *target_prop) + && !property_can_be_migrated_to_nullable(current_prop, *target_prop))) { table->remove_column(current_prop.table_column); ++deleted; current_prop.table_column = npos; From 0b45772a0b715d4ceb4c99aaae4835feda15158a Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Mon, 19 Oct 2015 18:53:09 -0700 Subject: [PATCH 3/3] Add a test showing our behavior when migrating from an optional column to a required column. Optional values are not automatically migrated to required columns since it is a lossy process. This test case revealed an issue where the number of objects can be lost if all properties of an object were optional and are all being migrated to required. This happens because the migration process removes the optional columns in a first pass, and recreates them as required in a second pass. Since this results in all columns being removed, we lose track of how many objects were stored. We avoid this by detecting the case where we are about to remove the last column and inserting a placeholder column that we'll remove after inserting the new columns. --- object_store.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/object_store.cpp b/object_store.cpp index 757d207f..ba7a868b 100644 --- a/object_store.cpp +++ b/object_store.cpp @@ -293,6 +293,8 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update changed = true; } + bool inserted_placeholder_column = false; + // remove extra columns size_t deleted = 0; for (auto& current_prop : current_schema.properties) { @@ -301,6 +303,13 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update auto target_prop = target_object_schema->property_for_name(current_prop.name); if (!target_prop || (property_has_changed(current_prop, *target_prop) && !property_can_be_migrated_to_nullable(current_prop, *target_prop))) { + if (deleted == current_schema.properties.size() - 1) { + // We're about to remove the last column from the table. Insert a placeholder column to preserve + // the number of rows in the table for the addition of new columns below. + table->add_column(type_Bool, "placeholder"); + inserted_placeholder_column = true; + } + table->remove_column(current_prop.table_column); ++deleted; current_prop.table_column = npos; @@ -336,6 +345,15 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update } } + if (inserted_placeholder_column) { + // We inserted a placeholder due to removing all columns from the table. Remove it, and update the indices + // of any columns that we inserted after it. + table->remove_column(0); + for (auto& target_prop : target_props) { + target_prop.table_column--; + } + } + // update table metadata if (target_object_schema->primary_key.length()) { // if there is a primary key set, check if it is the same as the old key