mirror of
https://github.com/milvus-io/milvus.git
synced 2026-07-21 02:05:41 +00:00
fix: preserve temp text index state across sealed segment Reopen (#49192)
issue: https://github.com/milvus-io/milvus/issues/49076 Signed-off-by: SpadeA <tangchenjie1210@gmail.com>
This commit is contained in:
@@ -2616,6 +2616,14 @@ ChunkedSegmentSealedImpl::CreateTextIndex(FieldId field_id,
|
||||
|
||||
std::unique_lock lck(mutex_);
|
||||
|
||||
// Guard against re-entry on a field whose temp text index was already
|
||||
// built: the path `<mmap>/<segment_id>_<field_id>` is shared with the
|
||||
// live holder in text_indexes_, and rebuilding there races with its
|
||||
// destructor's RemoveDir (issue #49076).
|
||||
AssertInfo(text_indexes_.find(field_id) == text_indexes_.end(),
|
||||
"text index for field {} already exists, refusing to rebuild",
|
||||
field_id.get());
|
||||
|
||||
const auto& field_meta = schema_->operator[](field_id);
|
||||
auto& cfg = storage::MmapManager::GetInstance().GetMmapConfig();
|
||||
std::unique_ptr<index::TextMatchIndex> index;
|
||||
@@ -2701,6 +2709,9 @@ ChunkedSegmentSealedImpl::CreateTextIndex(FieldId field_id,
|
||||
|
||||
text_indexes_[field_id] = std::make_shared<index::TextMatchIndexHolder>(
|
||||
std::move(index), cfg.GetScalarIndexEnableMmap());
|
||||
// Record under mutex_ together with text_indexes_ write — concurrent
|
||||
// readers of segment_load_info_ see a consistent view.
|
||||
segment_load_info_.SetTextIndexCreated(field_id);
|
||||
}
|
||||
|
||||
void
|
||||
@@ -3870,6 +3881,11 @@ ChunkedSegmentSealedImpl::Reopen(
|
||||
|
||||
std::unique_lock lck(mutex_);
|
||||
SegmentLoadInfo current(segment_load_info_);
|
||||
// Carry runtime-only state forward so subsequent Reopens don't
|
||||
// re-schedule CreateTextIndex on a field whose temp index was built.
|
||||
for (auto fid : current.GetCreatedTextIndexes()) {
|
||||
new_seg_load_info.SetTextIndexCreated(fid);
|
||||
}
|
||||
segment_load_info_ = new_seg_load_info;
|
||||
use_take_for_output_ = segment_load_info_.GetUseTakeForOutput();
|
||||
lck.unlock();
|
||||
@@ -4006,7 +4022,6 @@ ChunkedSegmentSealedImpl::ApplyLoadDiff(milvus::OpContext* op_ctx,
|
||||
if (!diff.text_indexes_to_create.empty()) {
|
||||
for (const auto& field_id : diff.text_indexes_to_create) {
|
||||
CreateTextIndex(field_id, op_ctx);
|
||||
segment_load_info.SetTextIndexCreated(field_id);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1291,6 +1291,13 @@ class ChunkedSegmentSealedImpl : public SegmentSealed {
|
||||
SetUseTakeForOutputForTesting(bool val) {
|
||||
use_take_for_output_ = val;
|
||||
}
|
||||
|
||||
// Test-only: direct access to segment_load_info_ for asserting Reopen
|
||||
// preserves runtime-only state (e.g. created_text_indexes_).
|
||||
SegmentLoadInfo&
|
||||
TestGetSegmentLoadInfo() {
|
||||
return segment_load_info_;
|
||||
}
|
||||
#endif
|
||||
|
||||
private:
|
||||
|
||||
@@ -830,6 +830,11 @@ class SegmentLoadInfo {
|
||||
created_text_indexes_.end();
|
||||
}
|
||||
|
||||
[[nodiscard]] const std::unordered_set<FieldId>&
|
||||
GetCreatedTextIndexes() const {
|
||||
return created_text_indexes_;
|
||||
}
|
||||
|
||||
// ==================== Diff Computation ====================
|
||||
|
||||
/**
|
||||
|
||||
@@ -70,6 +70,7 @@
|
||||
#include "segcore/SegcoreConfig.h"
|
||||
#include "segcore/SegmentGrowing.h"
|
||||
#include "segcore/SegmentGrowingImpl.h"
|
||||
#include "segcore/SegmentLoadInfo.h"
|
||||
#include "segcore/SegmentSealed.h"
|
||||
#include "segcore/Types.h"
|
||||
#include "storage/FileManager.h"
|
||||
@@ -2744,3 +2745,65 @@ TEST(SealedDropFieldData, PKFieldStillDropsBinlogIndex) {
|
||||
segment->DropFieldData(pk_id);
|
||||
EXPECT_TRUE(segment->HasFieldData(pk_id));
|
||||
}
|
||||
|
||||
// Reproducer for issue #49076.
|
||||
//
|
||||
// Reopen on a sealed segment does:
|
||||
// SegmentLoadInfo new_seg_load_info(new_load_info, schema_);
|
||||
// segment_load_info_ = new_seg_load_info; // <-- Bug 1
|
||||
// diff = current.ComputeDiff(new_seg_load_info);
|
||||
// ApplyLoadDiff(op_ctx, new_seg_load_info, diff); // <-- Bug 2
|
||||
//
|
||||
// `created_text_indexes_` is runtime-only (not proto-backed), so the RHS is
|
||||
// always empty; the copy-assign wipes the member (Bug 1). And any Create
|
||||
// done during this Reopen writes to the local new_seg_load_info, lost on
|
||||
// return (Bug 2). A later Reopen's ComputeDiff then re-schedules
|
||||
// CreateTextIndex for the same field at the same deterministic path,
|
||||
// racing with the still-live holder and causing
|
||||
// TantivyError: FileDoesNotExist("meta.json").
|
||||
TEST(SealedSegmentReopen, TextIndexCreatedWipedByReopen) {
|
||||
auto schema = std::make_shared<Schema>();
|
||||
schema->AddDebugField("pk", DataType::INT64); // 100
|
||||
schema->AddDebugField(
|
||||
"vec", DataType::VECTOR_FLOAT, 128, knowhere::metric::L2); // 101
|
||||
std::map<std::string, std::string> analyzer_params;
|
||||
schema->AddDebugVarcharField(FieldName("text_field"),
|
||||
DataType::VARCHAR,
|
||||
/*max_length=*/65535,
|
||||
/*nullable=*/false,
|
||||
/*enable_match=*/true,
|
||||
/*enable_analyzer=*/true,
|
||||
analyzer_params,
|
||||
std::nullopt); // 102
|
||||
schema->set_primary_field_id(FieldId(100));
|
||||
|
||||
const FieldId text_fid(102);
|
||||
|
||||
auto segment = CreateSealedSegment(schema);
|
||||
auto* sealed = dynamic_cast<ChunkedSegmentSealedImpl*>(segment.get());
|
||||
ASSERT_NE(sealed, nullptr);
|
||||
|
||||
proto::segcore::SegmentLoadInfo proto;
|
||||
proto.set_segmentid(49076);
|
||||
proto.set_num_of_rows(0);
|
||||
sealed->SetLoadInfo(proto);
|
||||
|
||||
// Simulate the state a prior Load would leave behind after running
|
||||
// CreateTextIndex for the enable_match field.
|
||||
sealed->TestGetSegmentLoadInfo().SetTextIndexCreated(text_fid);
|
||||
ASSERT_TRUE(sealed->TestGetSegmentLoadInfo().HasTextIndexCreated(text_fid));
|
||||
|
||||
// Reopen must preserve the runtime-only created_text_indexes_. Before
|
||||
// the fix this fails (member wiped by `segment_load_info_ =
|
||||
// new_seg_load_info`); after the fix it passes. The wipe happens before
|
||||
// ApplyLoadDiff, so even though ApplyLoadDiff throws here (num_rows_ is
|
||||
// unset because no real data was loaded), the state under test is
|
||||
// already settled.
|
||||
milvus::OpContext op_ctx;
|
||||
try {
|
||||
sealed->Reopen(&op_ctx, proto);
|
||||
} catch (...) {
|
||||
}
|
||||
|
||||
EXPECT_TRUE(sealed->TestGetSegmentLoadInfo().HasTextIndexCreated(text_fid));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user