Skip to content

Commit 7badfd9

Browse files
authored
Fix: prevent full-text inverting exceptions from deadlock (#3409)
**Issue** Full-text invert tasks run in ctpl worker threads via lambdas that increment counters (task_count_ / inflight_tasks_) before executing InvertColumn→Sort→ring_sorted_.Put. If any of these steps throws (e.g., simdjson INCORRECT_TYPE on empty JSON {}), the counter decrement / semaphore release code is never reached, causing the consumer to block forever. **Solution** Wrap all 4 inverting lambdas in try-catch: AsyncInsertBottom: --task_count_ + cv_.notify_one() always executes (outside try) AsyncInsert: on failure, release(sema) + --inflight_tasks_ Insert (offline/online): on failure, --inflight_tasks_ -- Make realtime the default for new full-text indexes
1 parent e8a9a26 commit 7badfd9

6 files changed

Lines changed: 276 additions & 49 deletions

File tree

src/storage/catalog/new_catalog_static_impl.cpp

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,69 @@ Status NewCatalog::MemIndexRecover(NewTxn *txn) {
339339
return status;
340340
}
341341
}
342+
343+
// Drain all pending fulltext index commits to ensure they complete before the checkpoint that follows.
344+
{
345+
auto DrainFulltextMemIndexes = [&](TableIndexMeta &table_index_meta) {
346+
auto [index_base, base_status] = table_index_meta.GetIndexBase();
347+
if (!base_status.ok() || index_base->index_type_ != IndexType::kFullText) {
348+
return;
349+
}
350+
auto [segment_ids_ptr, seg_status] = table_index_meta.GetSegmentIndexIDs1();
351+
if (!seg_status.ok()) {
352+
return;
353+
}
354+
for (SegmentID segment_id : *segment_ids_ptr) {
355+
SegmentIndexMeta segment_index_meta(segment_id, table_index_meta);
356+
std::shared_ptr<MemIndex> mem_index = segment_index_meta.GetMemIndex();
357+
if (mem_index == nullptr) {
358+
continue;
359+
}
360+
std::shared_ptr<MemoryIndexer> memory_indexer = mem_index->GetFulltextIndex();
361+
if (memory_indexer != nullptr) {
362+
memory_indexer->WaitForTaskCompletion();
363+
}
364+
}
365+
};
366+
367+
auto DrainTable = [&](TableMeta &table_meta) {
368+
std::vector<std::string> *index_id_strs_ptr = nullptr;
369+
std::vector<std::string> *index_name_strs_ptr = nullptr;
370+
Status st = table_meta.GetIndexIDs(index_id_strs_ptr, &index_name_strs_ptr);
371+
if (!st.ok()) {
372+
return;
373+
}
374+
for (size_t i = 0; i < index_id_strs_ptr->size(); ++i) {
375+
const std::string &index_id_str = (*index_id_strs_ptr)[i];
376+
const std::string &index_name_str = (*index_name_strs_ptr)[i];
377+
TableIndexMeta table_index_meta(index_id_str, index_name_str, table_meta);
378+
DrainFulltextMemIndexes(table_index_meta);
379+
}
380+
};
381+
382+
auto DrainDB = [&](DBMeta &db_meta) {
383+
std::vector<std::string> *table_id_strs_ptr = nullptr;
384+
std::vector<std::string> *table_names_ptr = nullptr;
385+
Status st = db_meta.GetTableIDs(table_id_strs_ptr, &table_names_ptr);
386+
if (!st.ok()) {
387+
return;
388+
}
389+
for (size_t i = 0; i < table_id_strs_ptr->size(); ++i) {
390+
const std::string &table_id_str = (*table_id_strs_ptr)[i];
391+
const std::string &table_name = (*table_names_ptr)[i];
392+
TableMeta table_meta(db_meta.db_id_str(), table_id_str, table_name, txn);
393+
DrainTable(table_meta);
394+
}
395+
};
396+
397+
for (size_t idx = 0; idx < db_count; ++idx) {
398+
const std::string &db_id_str = db_id_strs_ptr->at(idx);
399+
const std::string &db_name = db_names_ptr->at(idx);
400+
DBMeta db_meta(db_id_str, db_name, txn);
401+
DrainDB(db_meta);
402+
}
403+
}
404+
342405
return Status::OK();
343406
}
344407

src/storage/common/rcu_multimap.cppm

Lines changed: 67 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -618,6 +618,11 @@ private:
618618
InnerMap *volatile read_map_;
619619
std::size_t miss_time_;
620620

621+
// RCU reader count: incremented by readers before accessing read_map_,
622+
// decremented after the read is complete. Writers (CheckSwapInLock, CheckGc)
623+
// wait for this to reach 0 before freeing old maps, preventing use-after-free.
624+
mutable std::atomic<u64> rcu_reader_count_{0};
625+
621626
mutable std::mutex dirty_lock_;
622627
InnerMap *dirty_map_;
623628

@@ -662,6 +667,9 @@ typename RcuMap<Key, Value>::MapValue RcuMap<Key, Value>::CreateMapValue(const V
662667

663668
template <typename Key, typename Value>
664669
Value *RcuMap<Key, Value>::Get(const Key &key, bool update_access_time) {
670+
// RCU read protection: prevent write-side GC from freeing read_map_ during iteration
671+
rcu_reader_count_.fetch_add(1, std::memory_order_acquire);
672+
665673
// ULTRA-OPTIMIZED RCU READ PATTERN FOR READ-HEAVY WORKLOADS:
666674
// Eliminate ALL overhead in the common case to match MapWithLock performance
667675

@@ -686,9 +694,13 @@ Value *RcuMap<Key, Value>::Get(const Key &key, bool update_access_time) {
686694
}
687695
}
688696
}
697+
rcu_reader_count_.fetch_sub(1, std::memory_order_release);
689698
return &(it->second.value_);
690699
}
691700

701+
// RCU: temporarily release reader count while checking dirty_map under lock
702+
rcu_reader_count_.fetch_sub(1, std::memory_order_release);
703+
692704
// PHASE 2: Check dirty_map for recent writes (SLOW PATH - MINIMIZED)
693705
// This path should be rare in read-heavy workloads
694706
{
@@ -740,21 +752,25 @@ std::optional<Value> RcuMap<Key, Value>::GetValue(const Key &key, bool update_ac
740752

741753
template <typename Key, typename Value>
742754
Value *RcuMap<Key, Value>::GetWithRcuTime(const Key &key) {
755+
// RCU read protection
756+
rcu_reader_count_.fetch_add(1, std::memory_order_acquire);
757+
743758
// ABSOLUTE FASTEST PATH: Zero overhead reads for maximum performance
744-
// This should be as fast as MapWithLock::Get() with shared_lock
745759
InnerMap *current_read = read_map_;
746760
auto it = current_read->find(key);
747761

748762
if (it != current_read->end()) {
763+
rcu_reader_count_.fetch_sub(1, std::memory_order_release);
749764
return &(it->second.value_);
750765
}
751766

767+
rcu_reader_count_.fetch_sub(1, std::memory_order_release);
768+
752769
// Inline dirty_map check to avoid function call overhead
753770
{
754771
std::lock_guard<std::mutex> lock(dirty_lock_);
755772
auto dirty_it = dirty_map_->find(key);
756773
if (dirty_it != dirty_map_->end()) {
757-
// No access time updates, no miss tracking - pure read performance
758774
return &(dirty_it->second.value_);
759775
}
760776
}
@@ -764,18 +780,18 @@ Value *RcuMap<Key, Value>::GetWithRcuTime(const Key &key) {
764780

765781
template <typename Key, typename Value>
766782
Value *RcuMap<Key, Value>::GetReadOnly(const Key &key) {
767-
// BENCHMARK-OPTIMIZED READ: Absolute minimum overhead
768-
// This method is designed to match MapWithLock performance exactly
769-
// by eliminating ALL RCU-specific overhead
783+
// RCU read protection
784+
rcu_reader_count_.fetch_add(1, std::memory_order_acquire);
770785

771-
// Try read_map first (should succeed in most cases for read-heavy workloads)
772786
InnerMap *current_read = read_map_;
773787
auto it = current_read->find(key);
774788
if (it != current_read->end()) {
789+
rcu_reader_count_.fetch_sub(1, std::memory_order_release);
775790
return &(it->second.value_);
776791
}
777792

778-
// If not found, try dirty_map (minimal overhead)
793+
rcu_reader_count_.fetch_sub(1, std::memory_order_release);
794+
779795
std::lock_guard<std::mutex> lock(dirty_lock_);
780796
auto dirty_it = dirty_map_->find(key);
781797
if (dirty_it != dirty_map_->end()) {
@@ -826,6 +842,12 @@ void RcuMap<Key, Value>::CheckSwapInLock() {
826842
// std::set up new dirty_map for future writes
827843
dirty_map_ = new_dirty_map;
828844

845+
// RCU SYNCHRONIZATION: Wait for all readers that saw the OLD read_map_ to finish.
846+
// Without this, a reader could still be iterating the old map when CheckGc frees it.
847+
while (rcu_reader_count_.load(std::memory_order_acquire) > 0) {
848+
std::this_thread::yield();
849+
}
850+
829851
// Schedule old read_map for garbage collection
830852
// Cannot delete immediately - readers may still be using it
831853
deleted_map_list_.push_back(deleted_map);
@@ -929,6 +951,12 @@ void RcuMap<Key, Value>::CheckGc(u64 min_delete_time) {
929951
}
930952

931953
for (auto &deleted_map : map_need_delete) {
954+
// RCU: Wait for any in-flight readers to finish before freeing old maps.
955+
// CheckSwapInLock already waited, but there may be readers that started before
956+
// the swap and haven't finished yet.
957+
while (rcu_reader_count_.load(std::memory_order_acquire) > 0) {
958+
std::this_thread::yield();
959+
}
932960
delete deleted_map.deleted_entries_;
933961
delete deleted_map.map_;
934962
}
@@ -991,20 +1019,31 @@ void RcuMap<Key, Value>::emplace(const Key &key, Args &&...args) {
9911019

9921020
template <typename Key, typename Value>
9931021
u32 RcuMap<Key, Value>::range(const Key &key_min, const Key &key_max, std::vector<Value> &result) const {
994-
InnerMap *current_read = read_map_;
995-
auto read_begin = current_read->lower_bound(key_min);
996-
auto read_end = current_read->upper_bound(key_max);
997-
1022+
// 1. Acquire dirty snapshot under lock FIRST — avoids lock-ordering inversion
1023+
// where CheckSwapInLock holds dirty_lock_ and waits for readers to drain.
9981024
InnerMap *dirty_snapshot = nullptr;
9991025
{
10001026
std::lock_guard<std::mutex> lock(dirty_lock_);
10011027
dirty_snapshot = dirty_map_;
10021028
}
10031029

1030+
// 2. Now pin the reader count (read_map_ is stable)
1031+
rcu_reader_count_.fetch_add(1, std::memory_order_acquire);
1032+
1033+
// RAII: release reader count on any exit path (normal or exceptional)
1034+
struct RcuReaderGuard {
1035+
std::atomic<u64> &cnt_;
1036+
~RcuReaderGuard() { cnt_.fetch_sub(1, std::memory_order_release); }
1037+
} guard{rcu_reader_count_};
1038+
1039+
InnerMap *current_read = read_map_;
1040+
auto read_begin = current_read->lower_bound(key_min);
1041+
auto read_end = current_read->upper_bound(key_max);
1042+
10041043
auto dirty_begin = dirty_snapshot->lower_bound(key_min);
10051044
auto dirty_end = dirty_snapshot->upper_bound(key_max);
10061045

1007-
// merge - for std::map, we prioritize dirty_map values over read_map
1046+
// merge dirty_map values take priority over read_map
10081047
u32 count = 0;
10091048
auto read_it = read_begin;
10101049
auto dirty_it = dirty_begin;
@@ -1028,6 +1067,7 @@ u32 RcuMap<Key, Value>::range(const Key &key_min, const Key &key_max, std::vecto
10281067
}
10291068

10301069
return count;
1070+
// ~RcuReaderGuard releases reader count
10311071
}
10321072

10331073
// MapWithLock compatibility methods implementation
@@ -1081,16 +1121,26 @@ template <typename Key, typename Value>
10811121
void RcuMap<Key, Value>::Range(const Key &key_min, const Key &key_max, std::vector<std::pair<Key, Value>> &items) {
10821122
items.clear();
10831123

1084-
InnerMap *current_read = read_map_;
1085-
auto read_begin = current_read->lower_bound(key_min);
1086-
auto read_end = current_read->upper_bound(key_max);
1087-
1124+
// 1. Acquire dirty snapshot under lock FIRST — avoids lock-ordering inversion
10881125
InnerMap *dirty_snapshot = nullptr;
10891126
{
10901127
std::lock_guard<std::mutex> lock(dirty_lock_);
10911128
dirty_snapshot = dirty_map_;
10921129
}
10931130

1131+
// 2. Now pin the reader count (read_map_ is stable)
1132+
rcu_reader_count_.fetch_add(1, std::memory_order_acquire);
1133+
1134+
// RAII: release reader count on any exit path
1135+
struct RcuReaderGuard {
1136+
std::atomic<u64> &cnt_;
1137+
~RcuReaderGuard() { cnt_.fetch_sub(1, std::memory_order_release); }
1138+
} guard{rcu_reader_count_};
1139+
1140+
InnerMap *current_read = read_map_;
1141+
auto read_begin = current_read->lower_bound(key_min);
1142+
auto read_end = current_read->upper_bound(key_max);
1143+
10941144
auto dirty_begin = dirty_snapshot->lower_bound(key_min);
10951145
auto dirty_end = dirty_snapshot->upper_bound(key_max);
10961146

@@ -1109,6 +1159,7 @@ void RcuMap<Key, Value>::Range(const Key &key_min, const Key &key_max, std::vect
11091159
items.emplace_back(it->first, it->second.value_);
11101160
}
11111161
}
1162+
// ~RcuReaderGuard releases reader count
11121163
}
11131164

11141165
template <typename Key, typename Value>

src/storage/invertedindex/column_inverter.cppm

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,18 @@ public:
8181

8282
const std::vector<std::binary_semaphore *> &semas() const { return semas_; }
8383

84+
// Release all semaphores exactly once across all threads that process this inverter.
85+
void ReleaseSemas() {
86+
bool expected = false;
87+
if (!semas_released_.compare_exchange_strong(expected, true, std::memory_order_acq_rel)) {
88+
return;
89+
}
90+
for (auto *sema : semas_) {
91+
sema->release();
92+
}
93+
// semas_ does not need clearing since the atomic flag prevents re-entry.
94+
}
95+
8496
private:
8597
using TermBuffer = std::vector<char>;
8698
using PosInfoVec = std::vector<PosInfo>;
@@ -112,6 +124,7 @@ private:
112124

113125
u32 merged_{1};
114126
std::vector<std::binary_semaphore *> semas_{};
127+
std::atomic<bool> semas_released_{false};
115128

116129
protected:
117130
size_t InvertColumn(u32 doc_id, const std::string &val);

src/storage/invertedindex/index_defines.cppm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export {
2929
typedef u32 tf_t;
3030
typedef i64 ttf_t;
3131

32-
constexpr optionflag_t OPTION_FLAG_ALL = of_term_payload | of_doc_payload | of_position_list | of_term_frequency | of_block_max;
32+
constexpr optionflag_t OPTION_FLAG_ALL = of_term_payload | of_doc_payload | of_position_list | of_term_frequency | of_block_max | of_realtime;
3333
constexpr optionflag_t NO_BLOCK_MAX = of_term_payload | of_doc_payload | of_position_list | of_term_frequency;
3434
constexpr optionflag_t NO_TERM_FREQUENCY = of_term_payload | of_doc_payload;
3535
constexpr optionflag_t OPTION_FLAG_NONE = of_none;

0 commit comments

Comments
 (0)