Fixup for VisitedListHandle::check_and_update_visited #5427

xzfc · 2024-11-12T10:55:54Z

Fixup for #5336. In #5336, I made an assumption that the num_points provided to VisitedPool::get() is always accurate and removed bounds check from VisitedListHandle::check_and_update_visited.

let mut visited_list = visited_pool.get(num_points);
visited_list.check_and_update_visited(point_id); // assumption: point_id < num_points

But it seems that it's not true. Chaos testing shows that it crashes in the following code:

qdrant/lib/segment/src/index/struct_payload_index.rs

Lines 381 to 399 in ac54ad4

    
           let mut visited_list = self.visited_pool.get(id_tracker.total_point_count()); 
        
           let iter = query_cardinality 
        
               .primary_clauses 
        
               .iter() 
        
               .flat_map(|clause| { 
        
                   match clause { 
        
                       PrimaryCondition::Condition(field_condition) => { 
        
                           self.query_field(field_condition).unwrap_or_else( 
        
                               || id_tracker.iter_ids(), /* index is not built */ 
        
                           ) 
        
                       } 
        
                       PrimaryCondition::Ids(ids) => Box::new(ids.iter().copied()), 
        
                       PrimaryCondition::IsEmpty(_) => id_tracker.iter_ids(), /* there are no fast index for IsEmpty */ 
        
                       PrimaryCondition::IsNull(_) => id_tracker.iter_ids(),  /* no fast index for IsNull too */ 
        
                       PrimaryCondition::HasVector(_) => id_tracker.iter_ids(), /* no fast index for HasVector */ 
        
                   } 
        
               }) 
        
               .filter(move |&id| !visited_list.check_and_update_visited(id))

Perhaps the offending ids could come from PrimaryCondition::Ids/WAL/field_index? This PR restores the check.

Fixup for VisitedListHandle::check_and_update_visited

239b9bd

xzfc requested review from timvisee and generall November 12, 2024 10:55

timvisee approved these changes Nov 12, 2024

View reviewed changes

generall approved these changes Nov 12, 2024

View reviewed changes

generall merged commit 2958049 into dev Nov 12, 2024
17 checks passed

generall deleted the opt-visited-u8-fixup branch November 12, 2024 11:39

timvisee pushed a commit that referenced this pull request Nov 12, 2024

Fixup for VisitedListHandle::check_and_update_visited (#5427)

157d556

timvisee mentioned this pull request Nov 12, 2024

Bump version to 1.12.3 #5429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixup for VisitedListHandle::check_and_update_visited #5427

Fixup for VisitedListHandle::check_and_update_visited #5427

xzfc commented Nov 12, 2024

	let mut visited_list = self.visited_pool.get(id_tracker.total_point_count());

	let iter = query_cardinality
	.primary_clauses
	.iter()
	.flat_map(\|clause\| {
	match clause {
	PrimaryCondition::Condition(field_condition) => {
	self.query_field(field_condition).unwrap_or_else(
	\|\| id_tracker.iter_ids(), /* index is not built */
	)
	}
	PrimaryCondition::Ids(ids) => Box::new(ids.iter().copied()),
	PrimaryCondition::IsEmpty(_) => id_tracker.iter_ids(), /* there are no fast index for IsEmpty */
	PrimaryCondition::IsNull(_) => id_tracker.iter_ids(), /* no fast index for IsNull too */
	PrimaryCondition::HasVector(_) => id_tracker.iter_ids(), /* no fast index for HasVector */
	}
	})
	.filter(move \|&id\| !visited_list.check_and_update_visited(id))

Fixup for VisitedListHandle::check_and_update_visited #5427

Fixup for VisitedListHandle::check_and_update_visited #5427

Conversation

xzfc commented Nov 12, 2024