change impl of deleteAllIndexes

firebase · cherylEnkidu · Aug 29, 2023 · Jul 24, 2023 · Jul 28, 2023 · Jul 29, 2023
commit 6a8582b94aaa6b59910c4ce8bb3d2644bfc3c036
diff --git a/Firestore/Example/Tests/Integration/API/FIRIndexingTests.mm b/Firestore/Example/Tests/Integration/API/FIRIndexingTests.mm
@@ -109,8 +109,9 @@ - (void)testBadIndexDoesNotCrashClient {
  */
 - (void)testAutoIndexCreationSetSuccessfully {
   // Use persistent disk cache (explict)
-  FIRPersistentCacheSettings *cacheSettings = [FIRPersistentCacheSettings alloc];
-  self.db.settings.cacheSettings = cacheSettings;
+  FIRFirestoreSettings *settings = [self.db settings];
+  [settings setCacheSettings:[[FIRPersistentCacheSettings alloc] init]];
+  [self.db setSettings:settings];
 
   FIRCollectionReference *coll = [self collectionRef];
   NSDictionary *testDocs = @{

diff --git a/Firestore/core/src/local/index_manager.h b/Firestore/core/src/local/index_manager.h
@@ -112,8 +112,9 @@ class IndexManager {
   /** Returns all configured field indexes. */
   virtual std::vector<model::FieldIndex> GetFieldIndexes() const = 0;
 
-  /** Removes all field indexes and deletes all index values. */
-  virtual void DeleteAllFieldIndexes() = 0;
+  /** Removes all field indexes and deletes all index values. Function returns
+   * true is all deletions are finished, otherwise false. */
+  virtual bool DeleteAllFieldIndexes() = 0;
 
   /** Creates a full matched field index which serves the given target. */
   virtual void CreateTargetIndexes(const core::Target& target) = 0;

diff --git a/Firestore/core/src/local/leveldb_index_manager.cc b/Firestore/core/src/local/leveldb_index_manager.cc
@@ -464,27 +464,43 @@ absl::optional<model::FieldIndex> LevelDbIndexManager::GetFieldIndex(
   return result;
 }
 
-void LevelDbIndexManager::DeleteAllFieldIndexes() {
+bool LevelDbIndexManager::DeleteAllFieldIndexes() {
   HARD_ASSERT(started_, "IndexManager not started");
 
-  memoized_indexes_.clear();
-  next_index_to_update_ = QueueForNextIndexToUpdate();
-
-  db_->current_transaction()->DeleteEverythingWithPrefix(
+  bool is_finished = true;
+  is_finished = db_->current_transaction()->DeleteEverythingWithPrefix(
       "Delete All Index Configuration",
       LevelDbIndexConfigurationKey::KeyPrefix());
 
+  // Pause the task, creates another transaction to continue the work.
+  if (!is_finished) {
+    return false;
+  }
+
   // Delete states from all users for this index id.
-  db_->current_transaction()->DeleteEverythingWithPrefix(
+  is_finished = db_->current_transaction()->DeleteEverythingWithPrefix(
       "Delete All Index States", LevelDbIndexStateKey::KeyPrefix(),
       [](absl::string_view key) {
         LevelDbIndexStateKey state_key;
         return state_key.Decode(key);
       });
 
+  if (!is_finished) {
+    return false;
+  }
+
   // Delete entries from all users for this index id.
-  db_->current_transaction()->DeleteEverythingWithPrefix(
+  is_finished = db_->current_transaction()->DeleteEverythingWithPrefix(
       "Delete All Index Entries", LevelDbIndexEntryKey::KeyPrefix());
+
+  if (!is_finished) {
+    return false;
+  }
+
+  memoized_indexes_.clear();
+  next_index_to_update_ = QueueForNextIndexToUpdate();
+
+  return true;
 }
 
 void LevelDbIndexManager::CreateTargetIndexes(const core::Target& target) {

diff --git a/Firestore/core/src/local/leveldb_index_manager.h b/Firestore/core/src/local/leveldb_index_manager.h
@@ -69,7 +69,7 @@ class LevelDbIndexManager : public IndexManager {
 
   std::vector<model::FieldIndex> GetFieldIndexes() const override;
 
-  void DeleteAllFieldIndexes() override;
+  bool DeleteAllFieldIndexes() override;
 
   void CreateTargetIndexes(const core::Target& target) override;
 

diff --git a/Firestore/core/src/local/leveldb_transaction.cc b/Firestore/core/src/local/leveldb_transaction.cc
@@ -201,10 +201,12 @@ void LevelDbTransaction::Delete(absl::string_view key) {
   version_++;
 }
 
-void LevelDbTransaction::DeleteEverythingWithPrefix(
+bool LevelDbTransaction::DeleteEverythingWithPrefix(
     absl::string_view label,
     const std::string& prefix,
     const std::function<bool(absl::string_view key)> decode_function) {
+  bool is_finished = true;
+
   auto it = NewIterator();
   for (it->Seek(prefix); it->Valid() && absl::StartsWith(it->key(), prefix);
        it->Next()) {
@@ -216,12 +218,14 @@ void LevelDbTransaction::DeleteEverythingWithPrefix(
 
     if (changed_keys() >= 1000U) {
       LOG_DEBUG(
-          "Cannot delete all entries in transaction: %s due to the total count "
-          "of entries is more than or equal to 1000. Please trigger %s again.",
-          label, label);
+          "Cannot delete all entries in a single transaction. Please trigger "
+          "%s again.",
+          label);
+      is_finished = false;
       break;
     }
   }
+  return is_finished;
 }
 
 void LevelDbTransaction::Commit() {

diff --git a/Firestore/core/src/local/leveldb_transaction.h b/Firestore/core/src/local/leveldb_transaction.h
@@ -157,9 +157,11 @@ class LevelDbTransaction {
 
   /**
    * Remove the database entry (if any) for all "key" starting with given
-   * prefix.  It is not an error if "key" did not exist in the database.
+   * prefix.  It is not an error if "key" did not exist in the database. When
+   * function returns true, means the function has finished execution. If it
+   * returns false, function needs to be called again.
    */
-  void DeleteEverythingWithPrefix(
+  bool DeleteEverythingWithPrefix(
       absl::string_view label,
       const std::string& prefix,
       const std::function<bool(absl::string_view key)> decode_function = {});

diff --git a/Firestore/core/src/local/local_store.cc b/Firestore/core/src/local/local_store.cc
@@ -704,8 +704,9 @@ void LocalStore::SetIndexAutoCreationEnabled(bool is_enabled) const {
 }
 
 void LocalStore::DeleteAllFieldIndexes() const {
-  persistence_->Run("Delete All FieldIndexes",
-                    [&] { return index_manager_->DeleteAllFieldIndexes(); });
+  persistence_->RunUntilTaskFinished("Delete All FieldIndexes", [&] {
+    return index_manager_->DeleteAllFieldIndexes();
+  });
 }
 
 Target LocalStore::NewUmbrellaTarget(const std::string& bundle_id) {

diff --git a/Firestore/core/src/local/memory_index_manager.cc b/Firestore/core/src/local/memory_index_manager.cc
@@ -89,7 +89,10 @@ std::vector<model::FieldIndex> MemoryIndexManager::GetFieldIndexes() const {
   return {};
 }
 
-void MemoryIndexManager::DeleteAllFieldIndexes() {
+bool MemoryIndexManager::DeleteAllFieldIndexes() {
+  // No indexes for memory cache, so the function always returns true as
+  // finished.
+  return true;
 }
 
 void MemoryIndexManager::CreateTargetIndexes(const core::Target&) {

diff --git a/Firestore/core/src/local/memory_index_manager.h b/Firestore/core/src/local/memory_index_manager.h
@@ -67,7 +67,7 @@ class MemoryIndexManager : public IndexManager {
 
   std::vector<model::FieldIndex> GetFieldIndexes() const override;
 
-  void DeleteAllFieldIndexes() override;
+  bool DeleteAllFieldIndexes() override;
 
   void CreateTargetIndexes(const core::Target&) override;
 

diff --git a/Firestore/core/src/local/persistence.h b/Firestore/core/src/local/persistence.h
@@ -190,6 +190,26 @@ class Persistence {
     return result;
   }
 
+  /**
+   * Accepts a function and runs it within a transaction. When called, a
+   * transaction will be started before a block is run, and committed after the
+   * block has executed. Given there exists upper limit (1000 operations) for
+   * transaction, this task will run repeatedly until task is finished.
+   *
+   * @param label A semi-unique name for the transaction, for logging.
+   * @param block A function to be executed within the transaction whose return
+   *     value is boolean. When `block()` returns true, means the function has
+   * finished execution. If `block()` returns false, task needs to be executed
+   * again.
+   */
+  auto RunUntilTaskFinished(absl::string_view label,
+                            std::function<bool()> block) {
+    bool is_finished = false;
+    while (!is_finished) {
+      RunInternal(label, [&]() mutable { is_finished = block(); });
+    }
+  }
+
  private:
   virtual void RunInternal(absl::string_view label,
                            std::function<void()> block) = 0;

diff --git a/Firestore/core/test/unit/local/leveldb_local_store_test.cc b/Firestore/core/test/unit/local/leveldb_local_store_test.cc
@@ -689,6 +689,34 @@ TEST_F(LevelDbLocalStoreTest, DeleteAllIndexesWorksWithManualAddedIndexes) {
   FSTAssertQueryReturned("coll/a");
 }
 
+TEST_F(LevelDbLocalStoreTest, DeleteAllIndexesWorksWith500Documents) {
+  FieldIndex index =
+      MakeFieldIndex("coll", 0, FieldIndex::InitialState(), "matches",
+                     model::Segment::Kind::kAscending);
+  ConfigureFieldIndexes({index});
+
+  core::Query query =
+      testutil::Query("coll").AddingFilter(Filter("matches", "==", true));
+  int target_id = AllocateQuery(query);
+
+  for (int count = 1; count <= 500; count++) {
+    ApplyRemoteEvent(AddedRemoteEvent(
+        Doc("coll/" + std::to_string(count), 10, Map("matches", true)),
+        {target_id}));
+  }
+
+  SetBackfillerMaxDocumentsToProcess(500);
+  BackfillIndexes();
+
+  ExecuteQuery(query);
+  FSTAssertRemoteDocumentsRead(/* byKey= */ 500, /* byCollection= */ 0);
+
+  DeleteAllIndexes();
+
+  ExecuteQuery(query);
+  FSTAssertRemoteDocumentsRead(/* byKey= */ 0, /* byCollection= */ 500);
+}
+
 TEST_F(LevelDbLocalStoreTest, IndexAutoCreationWorksWithMutation) {
   core::Query query = testutil::Query("coll").AddingFilter(
       Filter("value", "array-contains-any", Array(8, 1, "string")));