fix(db): SIGSEGV when fetch/query race a segment switch; unblock reader concurrency - #715
fix(db): SIGSEGV when fetch/query race a segment switch; unblock reader concurrency#715YongqiYin wants to merge 10 commits into
Conversation
Fix a real data race: readers calling get_all_segments() (query / fetch / group_by_query / delete_by_filter / stats / prepare_iterate) read the writing_segment_ shared_ptr and doc_ids_ with no lock held in common with writers, which reassign writing_segment_ and push_back into doc_ids_ under exclusive write_mtx_. On Linux glibc this crashes under concurrent read+write load (7/7 runs: reader faults inside arrow::Table::FromRecord- Batches while the writer tears the same segment down via dump()); on macOS the same race silently corrupts results (probe hit rate 98.5%). Fix: take write_mtx_ shared inside get_all_segments() and add get_all_segments_unsafe() for callers that already hold it. Also remove the bottlenecks this exposed on the read path, so the fix does not trade correctness for throughput: - SegmentImpl::seg_mtx_ -> shared_mutex; Fetch()/get_global_doc_id() take it shared (read-only accessors) - MemForwardStore::cache_mtx_ -> shared_mutex; the 4 read-only accessors take it shared - MemForwardStore::convertToTable(): single-source fast path; a point fetch no longer rebuilds the whole in-memory store into an Arrow table (O(N) -> O(1), 500us -> 69us with 4k docs in buffer) Verification: 218 gtests green; 1.4M (macOS) + 2.1M (Linux glibc) field-by-field content-verified concurrent fetches, 0 errors; reader scaling 5.8-6.4x (1->8 readers, 64-core Linux); baseline crashes 7/7 vs fixed 3/3 green under the same load.
Guards the segment-list race fix and the newly-shared reader locks: one writer thread inserts (crossing frequent segment switches) while N reader threads fetch preloaded docs and compare every returned doc field by field against the deterministic expectation. Two load shapes are covered: with a writer the sealed segments serve reads from the persisted (mmap) path; without one every doc stays in MemForwardStore::cache_. max_doc_count_per_segment is deliberately small (4k): on the unfixed baseline reader contention starves the writer to ~2.5k docs/s, so a 40k threshold would produce no segment switch at all inside the run window - and the switch (dump(), which takes no seg_mtx_) is exactly where the baseline crashes. Verified on 64-core Linux glibc: crashes 3/3 on the pre-fix baseline (SIGSEGV while the writer switches segments); passes with the fix (774k field-by-field verified fetches, 0 errors). Also passes on macOS arm64 (489k fetches, 0 errors).
finish_memory_components() and init_memory_components() rewrite memory_store_, persist_stores_ and the vector indexer maps under the exclusive seg_mtx_, but the query read path reaches the same state under a different lock: fetch_normal()/scan() hold seg_col_mtx_ shared, and get_combined_vector_indexer() held no lock at all. Between memory_store_->close() and persist_stores_.push_back() neither branch in fetch_normal applies, so it skips the block and the query loses those rows -- no crash, no error, no log. With the window widened to 100ms, 12 of 16 queries dropped rows (88 total). Take seg_col_mtx_ exclusively in both rebuild paths and shared in the two combined-indexer accessors. The order stays seg_mtx_ -> seg_col_mtx_: flush() has three call sites (close, internal_insert, dump) and none of them holds seg_col_mtx_. After the fix, 0 rows dropped under the same conditions. From review, also in this commit: - dump() takes the exclusive seg_mtx_, covering Fetch(doc) and get_global_doc_id() against the same teardown. - MemForwardStore::close() takes the exclusive cache_mtx_, with flush_locked() split out so close() does not lock it twice. - The three convertToTable helpers renamed to snake_case. - New QuerySucceedsUnderConcurrentWrites test, asserting field completeness: the usual "no error / non-empty" checks stayed green through all 12 dropped-row events. Throughput stays within the +-1% noise floor (fetch -0.95%/+0.57%, query -0.53%, writer flat). Regression: 222 tests on macOS, 224 on Linux.
The file now covers both read entry points -- fetch() under seg_mtx_ and query() fanning out under seg_col_mtx_ -- so "fetch" in the name, the fixture and the header comment described only half of it. While renaming, two things in the query case were off: - It only checked that the requested field was present. Every field is derived from the doc id and the pk is "pk_<id>", so the value can be checked against the pk it came back with; that also catches rows stitched together across blocks, not just dropped ones. - Errors matching the known Insert/Query transient were skipped without a trace. They are now counted and printed separately, which shows the transient still fires on macOS (4 in 16k queries) but not on Linux.
Collapse seg_col_mtx_ into seg_mtx_ so overlapping segment state (memory_store_, persist_stores_, block metadata, indexer maps) is guarded by a single lock. Public entries lock; init/finish_memory_components and fetch_exec_unsafe run lock-free under the caller's lock; close() now takes the exclusive lock.
Keep this branch's single-lock segment design and short-lived collection lock: upstream alibaba#722 holds write_mtx_ shared for the whole query, which starves the exclusive writer on glibc (measured 6250 -> 12 docs/s with 4 readers, reproduced 3/3 on pure upstream code). Port the two indexer accessor locks added by upstream alibaba#726 (get_vector_indexer / get_quant_vector_indexer) so the merged lock covers everything seg_col_mtx_ did. Verified on Linux and macOS: 224 / 222 tests green, upstream's stricter Optimize concurrency test passes 5/5, query transients are zero, and fetch throughput is ~17x upstream with the writer running at full speed.
|
评审期间上游合入了 #722(synchronize reads of writing segment)和 #726(synchronize flat cache and
把两套方案在同一台机器上做了对照实测。 测试条件
结果
写者停滞的原因是读者饥饿:glibc 的 rwlock 读者优先,读者全程持 shared 正确性对齐#722 移除了 合并的处理保留本 PR 的锁设计,并吸收上游的覆盖面:补上 |
After replacing whole-query write_mtx_ with segment-level locking, meta reads outside seg_mtx_ were left racing Insert/flush: - add Segment::doc_id_range()/doc_count_snapshot() locked accessors; route the fetch segment search and DocFilter's brute-force threshold through them - make DeleteStore::empty_ atomic (get_filter() reads it lock-free per query) - document flush()/init/finish caller-held lock preconditions incl. the collection-level flush() path; fix stale comments and trim long ones
…consistency - Make Segment::doc_id_range()/doc_count_snapshot() pure virtual; the base default dereferenced meta() unconditionally, which is null for MockSegment. MockSegment now implements them explicitly. - Restore the shared write_mtx_ across delete_by_filter's scan so the matched set reflects one collection state (via get_all_segments_unsafe() to avoid recursively locking write_mtx_). Plain queries keep the lighter view.
| std::vector<Segment::Ptr> get_all_segments() const; | ||
|
|
||
| //! Same as get_all_segments(), for callers that already hold write_mtx_. | ||
| std::vector<Segment::Ptr> get_all_segments_unsafe() const; |
| std::shared_lock<std::shared_mutex> lock(seg_col_mtx_); | ||
| // Shared: finish_memory_components() migrates entries between these maps, | ||
| // so an unlocked read can miss an indexer or count it twice. | ||
| std::shared_lock<std::shared_mutex> lock(seg_mtx_); |
There was a problem hiding this comment.
[P1] The reader lock does not cover all writers of the segment metadata. In CollectionImpl::switch_to_new_segment_for_writing(), remove_writing_forward_block() (collection.cc:1730) runs after dump() has released seg_mtx_. A concurrent query can therefore pass has_writing_forward_block() in the CombinedVectorColumnIndexer constructor and race with the optional reset before value() or the BlockMeta copy. This is a data race with potential exceptions/crashes, not merely a weakly consistent read. Could we move this metadata removal into a Segment operation protected by the exclusive seg_mtx_?
Closes: #714
What
Both read entry points crash with SIGSEGV on Linux when a writer crosses a
segment switch (details in #714).
fetch()faults inside the in-memory tablerebuild;
query()faults on the same store, reached through the planner'sfan-out (
VectorRecallNode::collect_batch → SegmentImpl::fetch → fetch_normal → convertToTable). Two races and two read-path changes:get_all_segments()takeswrite_mtx_shared(
collection.cc), asstats()already does;_unsafevariant added forcallers already holding the lock. Probe on
main: 99.7% ofdoc_ids_push_backs executed while a reader was inside this function.
finish_memory_components()/init_memory_components()takeseg_col_mtx_exclusively, and the twocombined-vector-indexer accessors take it shared;
dump()takesseg_mtx_exclusively. Readers used to reach
memory_store_/persist_stores_/the indexer maps while
flush()was republishing them under a differentlock, so a reader landing between
memory_store_->close()andpersist_stores_.push_back()found neither and dropped that block's rowswith no error and no log. Locking the rebuild covers all three
flush()entry points (buffer-full insert, segment switch, close).
MemForwardStore::convertToTable()materializesonly the batch holding the requested rows instead of rebuilding the whole
in-memory store: O(store) → O(1), and it no longer walks the stack the
crash faults in.
seg_mtx_andcache_mtx_becomeshared_mutex;read-only accessors take them shared (verified member by member).
close()now takescache_mtx_exclusively, withflush_locked()extracted so it does not lock twice.
Also from review: the three
convertToTablehelpers moved to snake_case, andtests/db/concurrent_fetch_test.ccwas renamedconcurrent_read_test.ccnowthat it covers both read paths.
Test plan
tests/db/concurrent_read_test.cc, one writer crossing frequent segmentswitches throughout:
the deterministic generator output. 3/3 SIGSEGV on unfixed
main, passeswith the fix (2.4M field comparisons on Linux, 1.6M on macOS, 0 errors).
no empty results, no missing field values, and that each value matches the
pk it came back with. 3/3 SIGSEGV on unfixed
main, passes with the fix(19.5k queries on Linux, 16.5k on macOS, 0 errors).
Two settings are load-bearing:
max_doc_count_per_segment= 4,000(schema-validated ≥1,000) so switches happen inside the run window, and the
writer is capped at 50k docs and asserted to be alive (
writer_errorsplusa minimum
docs_written) — otherwise a starved writer silently turns the runinto a no-writer one and the test passes without exercising the crash path.
Full suites: 222 tests on macOS arm64, 224 on Linux x86_64.
Measured impact (64-core Linux; same-binary repeat variance <4%)
Known trade-offs
fetched the segment list with no lock at all — that unlocked read is exactly
the race being fixed. After it,
get_all_segments()takeswrite_mtx_shared, so a reader waits for the current
write_implbatch. This is theonly newly-blocked path at the collection layer:
create_index/add_columnalready blocked readers for the whole DDL task (they hold theschema lock exclusively), and per-insert exclusion already existed at the
segment layer.
for the duration of
dump()(avg 16ms, max 24ms, 26 occurrences over an8s window in the regression test) and of a memory-component rebuild.
Previously they were not blocked — they read state that was being torn down.
Measured cost is inside the ±1% run-to-run noise on every workload above.
std::shared_mutex, awriter waiting for a segment lock must wait out the readers currently inside
it. The wait stays bounded: new readers cannot arrive (the writer holds the
collection's exclusive
write_mtx_) and each in-flight read is now an O(1)lookup. Writer throughput stays flat as readers go 0→8 (−2.4%).