Skip to content

Improve Gridstore wipe/clear functionality #6614

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jun 2, 2025
Merged

Improve Gridstore wipe/clear functionality #6614

merged 6 commits into from
Jun 2, 2025

Conversation

timvisee
Copy link
Member

@timvisee timvisee commented May 30, 2025 •
edited
Loading

Gridstore has a wipe function which drops storage, it did take a &mut self reference. It was problematic.

It is fine to use to clear, close and remove storage. But, it's not fine to clear storage and reuse storage afterwards. It leaves the storage in an inconsistent state. I therefore consider this to be a nasty footgun, and I'd argue it must take ownership to disallow reuse.

I've added a test to show the problematic behavior. It calls wipe the same way as we do on other storages, producing this panic:

thread 'collection_manager::optimizers::indexing_optimizer::tests::test_indexing_optimizer' panicked at /home/timvisee/git/qdrant/lib/gridstore/src/gridstore.rs:369:39:
index out of bounds: the len is 0 but the index is 0
stack backtrace:
   0: __rustc::rust_begin_unwind
             at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/std/src/panicking.rs:697:5
   1: core::panicking::panic_fmt
             at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:75:14
   2: core::panicking::panic_bounds_check
             at /rustc/17067e9ac6d7ecb70e50f92c1944e545188d2359/library/core/src/panicking.rs:273:5
   3: <usize as core::slice::index::SliceIndex<[T]>>::index_mut
             at /home/timvisee/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:280:14
   4: index_mut<gridstore::page::Page, usize>
             at /home/timvisee/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/index.rs:27:9
   5: index_mut<gridstore::page::Page, usize, alloc::alloc::Global>
             at /home/timvisee/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/vec/mod.rs:3384:9
   6: write_into_pages<alloc::vec::Vec<i64, alloc::alloc::Global>>
             at /home/timvisee/git/qdrant/lib/gridstore/src/gridstore.rs:369:39
   7: gridstore::gridstore::Gridstore<V>::put_value
             at /home/timvisee/git/qdrant/lib/gridstore/src/gridstore.rs:451:9
   8: add_many_to_list<i64>
             at /home/timvisee/git/qdrant/lib/segment/src/index/field_index/numeric_index/mutable_numeric_index.rs:399:17
   ...

To fix the problem I've done two things:

  • make wipe(mut self) take ownership to disallow reuse
  • add clear(&mut self) to clear and get into the initial state (one empty page), allowing reuse

The clear function is naively implemented removing the directory and recreating storage, but I don't see this as a huge problem.

I actually discovered this problem while implementing #6609. It puzzled me for more than an hour, which is why I'm fixing it now.

All Submissions:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
  3. Have you checked your code using cargo clippy --all --all-features command?

Copilot

This comment was marked as resolved.

Sign in to view

@coderabbitai coderabbitai

This comment was marked as resolved.

Sign in to view
coderabbitai[bot]

This comment was marked as resolved.

Sign in to view

@timvisee timvisee force-pushed the gridstore-clear-and-wipe branch from d24185d to 19c3729 Compare May 30, 2025 09:40
@timvisee timvisee mentioned this pull request May 30, 2025
Merged
17 tasks
@KShivendu KShivendu self-requested a review May 30, 2025 18:33
@timvisee timvisee merged commit 845d03b into dev Jun 2, 2025
17 checks passed
@timvisee timvisee deleted the gridstore-clear-and-wipe branch June 2, 2025 11:18
@coderabbitai coderabbitai bot mentioned this pull request Jun 6, 2025
Merged
12 tasks
generall pushed a commit that referenced this pull request Jul 17, 2025
@timvisee @generall
Improve Gridstore wipe/clear functionality (#6614)
08194f9 
* Gridstore config can be Copy

* Add test asserting wipe behavior

* Add clear function to Gridstore

* Make wipe take ownership, use new clear function everywhere

* Add comment to emphasize wipe behavior

* Propagate errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Copilot code review Copilot Copilot left review comments

@agourlay agourlay agourlay approved these changes

@generall generall generall approved these changes

@JojiiOfficial JojiiOfficial JojiiOfficial approved these changes

@coszio coszio coszio approved these changes

@KShivendu KShivendu Awaiting requested review from KShivendu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@timvisee @agourlay @generall @JojiiOfficial @coszio