Skip to content

storage: clarify consistency as of when #158165

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −0
Open

storage: clarify consistency as of when #158165

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/storage/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,7 @@ type Reader interface {
visitSharedFile func(sst *pebble.SharedSSTMeta) error,
visitExternalFile func(sst *pebble.ExternalFile) error,
) error

// ConsistentIterators returns true if the Reader implementation guarantees
// that the different iterators constructed by this Reader will see the same
// underlying Engine state. This is not true about Batch writes: new iterators
Expand Down Expand Up @@ -963,6 +964,11 @@ type Engine interface {
// All iterators created from a read-only engine are guaranteed to provide a
// consistent snapshot of the underlying engine. See the comment on the
// Reader interface and the Reader.ConsistentIterators method.
//
// NB: consistent iterators are not as of when NewReader is called. They are
// as of the time when the first iterator is created (often, this is when the
// first read happens if iterators are not used directly), or earlier if
Copy link
Collaborator Author

@pav-kv pav-kv Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Looking for suggestion on how to word this. Iterators are always used, but I mean to say that they are often hidden behind things (e.g. MVCCGet), so it can be the "first read/access" from the storage package user's perspective.

Copy link
Collaborator

@sumeerbhola sumeerbhola Nov 21, 2025

Choose a reason for hiding this comment

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

I think we can leave it as the simple "when the first iterator is created, or earlier is Reader.PinEngineStateForIterators ...".
Code readers are expected to know that passing a Reader to some function could cause it to create an iterator.

// Reader.PinEngineStateForIterators is called.
NewReader(durability DurabilityRequirement) Reader
Copy link
Collaborator Author

@pav-kv pav-kv Nov 20, 2025

Choose a reason for hiding this comment

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

Any other places to call this out? NewReadOnly below seems like one.

Copy link
Collaborator

@sumeerbhola sumeerbhola Nov 21, 2025

Choose a reason for hiding this comment

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

We have many methods that give us a Reader: NewBatch, NewReader, NewReadOnly, NewUnindexedBatch.
We can either repeat the earlier comment with each. Or move that comment to the Reader declaration and point to it from all the methods. I have a preference for the latter.

// NewReadOnly returns a new instance of a ReadWriter that wraps this
// engine, and with the given durability requirement. This wrapper panics
Expand Down