From da805b73ea6d46566b59537cea1fe484c3715326 Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Thu, 24 Apr 2025 21:00:53 +0300 Subject: [PATCH 1/5] fix: RENAME in cluster mode crash --- src/server/search/doc_index.cc | 5 ++++- src/server/search/search_family_test.cc | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/server/search/doc_index.cc b/src/server/search/doc_index.cc index e2975bab687e..5721f1654001 100644 --- a/src/server/search/doc_index.cc +++ b/src/server/search/doc_index.cc @@ -191,7 +191,10 @@ string DocIndexInfo::BuildRestoreCommand() const { } ShardDocIndex::DocId ShardDocIndex::DocKeyIndex::Add(string_view key) { - DCHECK_EQ(ids_.count(key), 0u); + auto it = ids_.find(key); + if (it != ids_.end()) { + return it->second; + } DocId id; if (!free_ids_.empty()) { diff --git a/src/server/search/search_family_test.cc b/src/server/search/search_family_test.cc index 82fbc3e71012..acc72f3b468e 100644 --- a/src/server/search/search_family_test.cc +++ b/src/server/search/search_family_test.cc @@ -2722,4 +2722,21 @@ TEST_F(SearchFamilyTest, JsonWithNullFields) { AreDocIds("doc:1", "doc:2")); } +TEST_F(SearchFamilyTest, RenameDocumentBetweenIndices) { + SetTestFlag("cluster_mode", "emulated"); + ResetService(); + + EXPECT_EQ(Run({"ft.create", "idx1", "prefix", "1", "idx1", "filter", "@index==\"yes\"", "schema", + "t", "text"}), + "OK"); + EXPECT_EQ(Run({"ft.create", "idx2", "prefix", "1", "idx2", "filter", "@index==\"yes\"", "schema", + "t", "text"}), + "OK"); + + Run({"hset", "idx1:{doc}1", "t", "foo1", "index", "yes"}); + + EXPECT_EQ(Run({"rename", "idx1:{doc}1", "idx2:{doc}1"}), "OK"); + EXPECT_EQ(Run({"rename", "idx2:{doc}1", "idx1:{doc}1"}), "OK"); +} + } // namespace dfly From 44bedb192149f47b01582b8a1aa0bf99b6a3e634 Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Thu, 24 Apr 2025 21:09:56 +0300 Subject: [PATCH 2/5] fix: test was updated --- src/server/search/search_family_test.cc | 5 +++++ src/server/test_utils.cc | 6 ++++++ src/server/test_utils.h | 1 + 3 files changed, 12 insertions(+) diff --git a/src/server/search/search_family_test.cc b/src/server/search/search_family_test.cc index acc72f3b468e..0e3d934cd340 100644 --- a/src/server/search/search_family_test.cc +++ b/src/server/search/search_family_test.cc @@ -2723,6 +2723,8 @@ TEST_F(SearchFamilyTest, JsonWithNullFields) { } TEST_F(SearchFamilyTest, RenameDocumentBetweenIndices) { + std::string original_mode = GetTestFlag("cluster_mode"); + SetTestFlag("cluster_mode", "emulated"); ResetService(); @@ -2737,6 +2739,9 @@ TEST_F(SearchFamilyTest, RenameDocumentBetweenIndices) { EXPECT_EQ(Run({"rename", "idx1:{doc}1", "idx2:{doc}1"}), "OK"); EXPECT_EQ(Run({"rename", "idx2:{doc}1", "idx1:{doc}1"}), "OK"); + + SetTestFlag("cluster_mode", original_mode); + ResetService(); } } // namespace dfly diff --git a/src/server/test_utils.cc b/src/server/test_utils.cc index 8fe514591e87..e06ff5e10107 100644 --- a/src/server/test_utils.cc +++ b/src/server/test_utils.cc @@ -754,6 +754,12 @@ void BaseFamilyTest::SetTestFlag(string_view flag_name, string_view new_value) { CHECK(flag->ParseFrom(new_value, &error)) << "Error: " << error; } +std::string BaseFamilyTest::GetTestFlag(string_view flag_name) { + auto* flag = absl::FindCommandLineFlag(flag_name); + CHECK_NE(flag, nullptr); + return flag->CurrentValue(); +} + const acl::AclFamily* BaseFamilyTest::TestInitAclFam() { absl::SetFlag(&FLAGS_acllog_max_len, 0); return service_->TestInit(); diff --git a/src/server/test_utils.h b/src/server/test_utils.h index b15ff146038c..f65e63847820 100644 --- a/src/server/test_utils.h +++ b/src/server/test_utils.h @@ -153,6 +153,7 @@ class BaseFamilyTest : public ::testing::Test { static unsigned NumLocked(); static void SetTestFlag(std::string_view flag_name, std::string_view new_value); + static std::string GetTestFlag(std::string_view flag_name); const acl::AclFamily* TestInitAclFam(); From 96c58e170da3ea53441a02b67ab1e9df4e448e58 Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Thu, 24 Apr 2025 21:34:26 +0300 Subject: [PATCH 3/5] fix: test was updated --- src/server/search/search_family_test.cc | 5 +---- src/server/test_utils.cc | 6 ------ src/server/test_utils.h | 1 - 3 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/server/search/search_family_test.cc b/src/server/search/search_family_test.cc index 0e3d934cd340..00287c3c11d8 100644 --- a/src/server/search/search_family_test.cc +++ b/src/server/search/search_family_test.cc @@ -2723,7 +2723,7 @@ TEST_F(SearchFamilyTest, JsonWithNullFields) { } TEST_F(SearchFamilyTest, RenameDocumentBetweenIndices) { - std::string original_mode = GetTestFlag("cluster_mode"); + absl::FlagSaver fs; SetTestFlag("cluster_mode", "emulated"); ResetService(); @@ -2739,9 +2739,6 @@ TEST_F(SearchFamilyTest, RenameDocumentBetweenIndices) { EXPECT_EQ(Run({"rename", "idx1:{doc}1", "idx2:{doc}1"}), "OK"); EXPECT_EQ(Run({"rename", "idx2:{doc}1", "idx1:{doc}1"}), "OK"); - - SetTestFlag("cluster_mode", original_mode); - ResetService(); } } // namespace dfly diff --git a/src/server/test_utils.cc b/src/server/test_utils.cc index e06ff5e10107..8fe514591e87 100644 --- a/src/server/test_utils.cc +++ b/src/server/test_utils.cc @@ -754,12 +754,6 @@ void BaseFamilyTest::SetTestFlag(string_view flag_name, string_view new_value) { CHECK(flag->ParseFrom(new_value, &error)) << "Error: " << error; } -std::string BaseFamilyTest::GetTestFlag(string_view flag_name) { - auto* flag = absl::FindCommandLineFlag(flag_name); - CHECK_NE(flag, nullptr); - return flag->CurrentValue(); -} - const acl::AclFamily* BaseFamilyTest::TestInitAclFam() { absl::SetFlag(&FLAGS_acllog_max_len, 0); return service_->TestInit(); diff --git a/src/server/test_utils.h b/src/server/test_utils.h index f65e63847820..b15ff146038c 100644 --- a/src/server/test_utils.h +++ b/src/server/test_utils.h @@ -153,7 +153,6 @@ class BaseFamilyTest : public ::testing::Test { static unsigned NumLocked(); static void SetTestFlag(std::string_view flag_name, std::string_view new_value); - static std::string GetTestFlag(std::string_view flag_name); const acl::AclFamily* TestInitAclFam(); From ac243afb30bc9987f5dd68862ed4055aa4cc6f41 Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Sun, 27 Apr 2025 14:11:32 +0300 Subject: [PATCH 4/5] fix: review comments were fixed --- src/server/generic_family.cc | 3 +++ src/server/search/doc_index.cc | 5 +---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/server/generic_family.cc b/src/server/generic_family.cc index 97c03eb96b6a..67f74857ab20 100644 --- a/src/server/generic_family.cc +++ b/src/server/generic_family.cc @@ -870,6 +870,9 @@ OpResult OpRen(const OpArgs& op_args, string_view from_key, string_view to is_prior_list = (to_res.it->second.ObjType() == OBJ_LIST); } + // Delete the document from the search index before deleting from the database + op_args.shard->search_indices()->RemoveDoc(from_key, op_args.db_cntx, from_res.it->second); + bool sticky = from_res.it->first.IsSticky(); uint64_t exp_ts = db_slice.ExpireTime(from_res.exp_it); diff --git a/src/server/search/doc_index.cc b/src/server/search/doc_index.cc index 5721f1654001..e2975bab687e 100644 --- a/src/server/search/doc_index.cc +++ b/src/server/search/doc_index.cc @@ -191,10 +191,7 @@ string DocIndexInfo::BuildRestoreCommand() const { } ShardDocIndex::DocId ShardDocIndex::DocKeyIndex::Add(string_view key) { - auto it = ids_.find(key); - if (it != ids_.end()) { - return it->second; - } + DCHECK_EQ(ids_.count(key), 0u); DocId id; if (!free_ids_.empty()) { From 74af6a6d610ce080fb8046672f6c8ff9fa0b15cc Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Mon, 28 Apr 2025 10:49:55 +0300 Subject: [PATCH 5/5] Update src/server/generic_family.cc Co-authored-by: Roman Gershman Signed-off-by: Volodymyr Yavdoshenko --- src/server/generic_family.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/generic_family.cc b/src/server/generic_family.cc index 67f74857ab20..fb7e9677b94e 100644 --- a/src/server/generic_family.cc +++ b/src/server/generic_family.cc @@ -870,7 +870,7 @@ OpResult OpRen(const OpArgs& op_args, string_view from_key, string_view to is_prior_list = (to_res.it->second.ObjType() == OBJ_LIST); } - // Delete the document from the search index before deleting from the database + // Delete the "from" document from the search index before deleting from the database op_args.shard->search_indices()->RemoveDoc(from_key, op_args.db_cntx, from_res.it->second); bool sticky = from_res.it->first.IsSticky();