Skip to content

fix(server): Process empty buckets in snapshot#7185

Merged
dranikpg merged 1 commit intodragonflydb:mainfrom
dranikpg:update-iterator-done
Apr 23, 2026
Merged

fix(server): Process empty buckets in snapshot#7185
dranikpg merged 1 commit intodragonflydb:mainfrom
dranikpg:update-iterator-done

Conversation

@dranikpg
Copy link
Copy Markdown
Contributor

@dranikpg dranikpg commented Apr 19, 2026

  1. Allow "done" (aka end) iterators to support GetVersion calls by keeping the dash pointer and adding a done_ field
  2. Allow including empty buckets in TraverseBuckets
  3. Stamp snapshot versions on empty buckets

Needed for #7060

Currently, the comparison bucket_version < snapsho_version determines whether the snapshot serialization process has reached this specific bucket. This fact can be later used to make decisions and perform various optimizations (for example journal omits, see #7060).

However, empty buckets are currently fully ignored by all actors: the dashtable iteration functions, the iterator itself and the snapshot. During serialization, their versions are not updated - because there is nothing to serialize. However, just by looking at the bucket and it's not-updated version, we can't tell if the snapshot loop has already reached it or will do so only in the future.

This PR allows them in all places, so they act as regular buckets and their versions are updated

@dranikpg
Copy link
Copy Markdown
Contributor Author

augment review

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Apr 20, 2026

🤖 Augment PR Summary

Summary: Ensures snapshot traversal also processes empty physical buckets so they get version-stamped and aren’t repeatedly treated as “unprocessed” (fix for #7060).

Changes:

  • Extend DashTable::Iterator with a done_ flag so “done” iterators can keep bucket context and still support GetVersion/SetVersion.
  • Add an optional visit_empty flag to DashTable::TraverseBuckets to invoke the callback for empty buckets.
  • Update snapshot traversal to request empty-bucket visitation during baseline scanning.
  • Stamp snapshot versions on empty buckets and flush tiered-storage delayed entries for touched buckets even when the iterator is “done”.

Technical Notes: Empty-bucket version stamping prevents repeated skip/rehit behavior during snapshotting and change-listener updates.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

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

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

@dranikpg dranikpg requested review from kostasrim and romange April 20, 2026 07:13
@dranikpg dranikpg marked this pull request as ready for review April 20, 2026 07:13
Copilot AI review requested due to automatic review settings April 20, 2026 07:13
Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

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

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates snapshot bucket traversal/versioning so that empty buckets are also processed and stamped with the snapshot version, enabling correctness needed for journal-omit behavior (#7060).

Changes:

  • Extend DashTable::TraverseBuckets with an option to visit empty buckets.
  • Change DashTable iterators to retain the owning table pointer even when “done”, via a done_ flag.
  • Update snapshot serialization flow to traverse empty buckets and stamp snapshot versions on them.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/server/snapshot.cc Traverses buckets with include empty buckets enabled so empty buckets are processed during snapshotting.
src/server/serializer_base.cc Stamps snapshot versions onto empty buckets and ensures delayed tiered entries are flushed when appropriate.
src/core/dash.h Adds visit_empty option to TraverseBuckets and switches iterator “done” tracking from null-owner to an explicit done_ flag.

@dranikpg
Copy link
Copy Markdown
Contributor Author

I haven't included migrations intentionally, I will first implement omits for snapshots fully and then transfer this knowledge to migrations

@romange
Copy link
Copy Markdown
Collaborator

romange commented Apr 20, 2026

For sake of discussion, if the bucket is empty and serialization have reached it, we must emit a journal change.
If it have not reached it, and i'ts empty, we may emit a journal change, and then the subsequent traversal will omit it as its version will be greater than the snapshot version.

So in either case we if emit a journal change when encountering an empty bucket in OnDbChange will behave correctly even if we do not update the bucket version. Am I wrong?

@dranikpg
Copy link
Copy Markdown
Contributor Author

For sake of discussion, if the bucket is empty and serialization have reached it, we must emit a journal change.
If it have not reached it, and i'ts empty, we may emit a journal change, and then the subsequent traversal will omit it as its version will be greater than the snapshot version.

Yes, we may emit a journal change. And it might look harmless because there is no difference when to serialize this single value.

But we actually don't want to do it. It is because it increases the "attack" surface - any subsequent write to that bucket now has to be journaled. Compare that with omitting this bucket - no writes will be journaled.

@dranikpg dranikpg merged commit b5de506 into dragonflydb:main Apr 23, 2026
17 checks passed
@dranikpg dranikpg deleted the update-iterator-done branch April 23, 2026 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants