Merge pull request #4456 from realm/tg/query-rerun

Fix the check for if ResultsNotifier needs to rerun queries

CHANGELOG.md

@@ -7,6 +7,7 @@
 * <How to hit and notice issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
 * Support upgrading from file format 5. ([#7089](https://github.com/realm/realm-cocoa/issues/7089), since v6.0.0)
 * On 32bit devices you may get exception with "No such object" when upgrading to v10.* ([#7314](https://github.com/realm/realm-java/issues/7314), since v10.0.0)
+* The notification worker thread would rerun queries after every commit rather than only commits which modified tables which could effect the query results if the table had any outgoing links to tables not used in the query (since v6.0.0).
 
 ### Breaking changes
 * None.

src/realm/object-store/impl/collection_notifier.cpp

@@ -27,24 +27,16 @@
 using namespace realm;
 using namespace realm::_impl;
 
-bool CollectionNotifier::all_related_tables_covered(const TableVersions& versions)
+bool CollectionNotifier::any_related_table_was_modified(TransactionChangeInfo const& info) const noexcept
 {
-    if (m_related_tables.size() > versions.size()) {
-        return false;
-    }
-    auto first = versions.begin();
-    auto last = versions.end();
-    for (auto& it : m_related_tables) {
-        TableKey tk{it.table_key};
-        auto match = std::find_if(first, last, [tk](auto& elem) {
-            return elem.first == tk;
-        });
-        if (match == last) {
-            // tk not found in versions
-            return false;
-        }
-    }
-    return true;
+    // Check if any of the tables accessible from the root table were
+    // actually modified. This can be false if there were only insertions, or
+    // deletions which were not linked to by any row in the linking table
+    auto table_modified = [&](auto& tbl) {
+        auto it = info.tables.find(tbl.table_key.value);
+        return it != info.tables.end() && !it->second.modifications_empty();
+    };
+    return any_of(begin(m_related_tables), end(m_related_tables), table_modified);
 }
 
 std::function<bool(ObjectChangeSet::ObjectKeyType)>
@@ -53,18 +45,12 @@ CollectionNotifier::get_modification_checker(TransactionChangeInfo const& info,
     if (info.schema_changed)
         set_table(root_table);
 
-    // First check if any of the tables accessible from the root table were
-    // actually modified. This can be false if there were only insertions, or
-    // deletions which were not linked to by any row in the linking table
-    auto table_modified = [&](auto& tbl) {
-        auto it = info.tables.find(tbl.table_key.value);
-        return it != info.tables.end() && !it->second.modifications_empty();
-    };
-    if (!any_of(begin(m_related_tables), end(m_related_tables), table_modified)) {
+    if (!any_related_table_was_modified(info)) {
         return [](ObjectChangeSet::ObjectKeyType) {
             return false;
         };
     }
+
     if (m_related_tables.size() == 1) {
         auto& object_set = info.tables.find(m_related_tables[0].table_key.value)->second;
         return [&](ObjectChangeSet::ObjectKeyType object_key) {

src/realm/object-store/impl/collection_notifier.hpp

@@ -205,7 +205,7 @@ class CollectionNotifier {
     // but only report this to the notifiers the first time this is reported
     void report_collection_root_is_deleted();
 
-    bool all_related_tables_covered(const TableVersions& versions);
+    bool any_related_table_was_modified(TransactionChangeInfo const&) const noexcept;
     std::function<bool(ObjectChangeSet::ObjectKeyType)> get_modification_checker(TransactionChangeInfo const&,
                                                                                  ConstTableRef);
 

src/realm/object-store/impl/results_notifier.cpp

@@ -102,27 +102,6 @@ bool ResultsNotifier::do_add_required_change_info(TransactionChangeInfo& info)
     return m_query->get_table() && has_run() && have_callbacks();
 }
 
-bool ResultsNotifier::need_to_run()
-{
-    REALM_ASSERT(m_info);
-
-    {
-        auto lock = lock_target();
-        // Don't run the query if the results aren't actually going to be used
-        if (!get_realm() || (!have_callbacks() && !m_results_were_used))
-            return false;
-    }
-
-    // If we've run previously, check if we need to rerun
-    if (has_run() && m_query->sync_view_if_needed() == m_last_seen_version) {
-        // Does m_last_seen_version match m_related_tables
-        if (all_related_tables_covered(m_last_seen_version)) {
-            return false;
-        }
-    }
-    return true;
-}
-
 void ResultsNotifier::calculate_changes()
 {
     if (has_run() && have_callbacks()) {
@@ -156,6 +135,8 @@ void ResultsNotifier::calculate_changes()
 
 void ResultsNotifier::run()
 {
+    REALM_ASSERT(m_info);
+
     // Table's been deleted, so report all rows as deleted
     if (!m_query->get_table()) {
         m_change = {};
@@ -164,14 +145,37 @@ void ResultsNotifier::run()
         return;
     }
 
-    if (!need_to_run())
+    {
+        auto lock = lock_target();
+        // Don't run the query if the results aren't actually going to be used
+        if (!get_realm() || (!have_callbacks() && !m_results_were_used))
+            return;
+    }
+
+    auto new_versions = m_query->sync_view_if_needed();
+    m_descriptor_ordering.collect_dependencies(m_query->get_table().unchecked_ptr());
+    m_descriptor_ordering.get_versions(m_query->get_table()->get_parent_group(), new_versions);
+    if (has_run() && new_versions == m_last_seen_version) {
+        // We've run previously and none of the tables involved in the query
+        // changed so we don't need to rerun the query, but we still need to
+        // check each object in the results to see if it was modified
+        if (!any_related_table_was_modified(*m_info))
+            return;
+        REALM_ASSERT(m_change.empty());
+        auto checker = get_modification_checker(*m_info, m_query->get_table());
+        for (size_t i = 0; i < m_previous_rows.size(); ++i) {
+            if (checker(m_previous_rows[i])) {
+                m_change.modifications.add(i);
+            }
+        }
         return;
+    }
 
     m_query->sync_view_if_needed();
     m_run_tv = m_query->find_all();
     m_run_tv.apply_descriptor_ordering(m_descriptor_ordering);
     m_run_tv.sync_if_needed();
-    m_last_seen_version = m_run_tv.ObjList::get_dependency_versions();
+    m_last_seen_version = std::move(new_versions);
 
     calculate_changes();
 }

src/realm/object-store/impl/results_notifier.hpp

@@ -70,7 +70,6 @@ class ResultsNotifier : public ResultsNotifierBase {
     TransactionChangeInfo* m_info = nullptr;
     bool m_results_were_used = true;
 
-    bool need_to_run();
     void calculate_changes();
 
     void run() override;