Skip to content

Improve Gridstore wipe/clear functionality#6614

Merged
timvisee merged 6 commits intodevfrom
gridstore-clear-and-wipe
Jun 2, 2025
Merged

Improve Gridstore wipe/clear functionality#6614
timvisee merged 6 commits intodevfrom
gridstore-clear-and-wipe

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?

This comment was marked as resolved.

Sign in to view

@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
@coderabbitai coderabbitai bot mentioned this pull request Nov 27, 2025
Merged
This was referenced Dec 5, 2025
Merged
Merged
@coderabbitai coderabbitai bot mentioned this pull request Dec 16, 2025
Merged
@coderabbitai coderabbitai bot mentioned this pull request Mar 3, 2026
Merged
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.

6 participants

@timvisee @agourlay @generall @JojiiOfficial @coszio