Skip to content

Conversation

timvisee
Copy link
Member

@timvisee timvisee commented Jul 31, 2025

Fix flushing behavior in the dynamic mmap flags structure, affecting the null and boolean indices.

This makes two changes:

  1. on flush, directly flush flags (don't use single-use flusher that noop's on the next flush)
  2. before resizing mmap, explicitly flush dirty pages (looks like we lose dirty pages if we don't)

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?

Comment on lines 171 to 172
// Explicitly flush so we don't lose dirty pages
self.flusher()()?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • because all set functions require &mut we are guaranteed to have exclusive access here, we're therefore safe to flush
  • we now flush both flags and the status file, we probably only need to flush flags

Note that we should assume that we can flush this structure at any time. New writes to the bool/null index should not be written to this structure directly and should be deferred until we flush, that will be implemented in a separate PR.

@timvisee
Copy link
Member Author

timvisee commented Aug 4, 2025

Let's finalize and merge https://github.com/qdrant/qdrant/pull/6954/files first, and see what parts of this remain relevant.

Copy link
Contributor

@coszio coszio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commited a change which flushes flags only before reopening the mmap

@coszio coszio mentioned this pull request Aug 4, 2025
@timvisee
Copy link
Member Author

timvisee commented Aug 5, 2025

Note: we should still merge this to satisfy #6954 (comment)

@timvisee timvisee added the next patch Include this in the next patch release label Aug 5, 2025
@timvisee timvisee force-pushed the fix-null-index-flushing branch from 77dff49 to c596a83 Compare August 5, 2025 12:47
@timvisee timvisee marked this pull request as ready for review August 5, 2025 13:01

This comment was marked as resolved.

@timvisee timvisee merged commit 6a116a5 into dev Aug 5, 2025
16 checks passed
@timvisee timvisee deleted the fix-null-index-flushing branch August 5, 2025 15:05
timvisee added a commit that referenced this pull request Aug 11, 2025
* Directly flush flags, don't use single-use flusher

* Explicitly flush before resizing flags file, prevent losing dirty pages

* Don't create now obsolete flusher when opening mmap

* clippy

* Move flush call

* flush before resizing only

---------

Co-authored-by: Luis Cossío <luis.cossio@outlook.com>
@timvisee timvisee mentioned this pull request Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next patch Include this in the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants