storage: add Reader method to pin iterators at current engine state #66845
Conversation
|
This change is |
There was a problem hiding this comment.
You don't seem to actually have made the change to switch reads to use the new pinning. Why not? It'd be good to do it in this PR, I'd say.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @itsbilal, @nvanbenschoten, and @sumeerbhola)
pkg/kv/kvserver/spanset/batch.go, line 486 at r1 (raw file):
} func (s spanSetReader) PinEngineStateForIterators() error {
it's a wide-spread standard in the codebase to put a comment saying PingEngine... is part of the storage.Reader interace`. I see that it's not done around here, but consider starting to do it; I find these comments quite useful.
pkg/storage/engine.go, line 367 at r1 (raw file):
// - pebbleReadOnly, pebbleBatch: when the IterOptions do not specify a // timestamp hint. Note that currently the engine state visible here is // not as of the time of the Reader creation. It is the time when the
does this comment need updating?
pkg/storage/engine.go, line 447 at r1 (raw file):
// ConsistentIterators returns true if the Reader implementation guarantees // that the different iterators constructed by this Reader will see the // same underlying Engine state. NB: this only applies to iterators without
This concept of a "timestamp hint" appears now 3 times in the comments here, but it's not explained anywhere that I can see. Should it be?
pkg/storage/engine.go, line 447 at r1 (raw file):
// ConsistentIterators returns true if the Reader implementation guarantees // that the different iterators constructed by this Reader will see the // same underlying Engine state. NB: this only applies to iterators without
what does it mean for "this" to "not apply" to some iterators? Does that mean that those iterators will still be "inconsistent" even when ConsistentIterators returns true? Consider expanding the comment.
pkg/storage/engine.go, line 453 at r1 (raw file):
will not see future mutations. The exception is the implementation that
It sounds to me like, technically, the Engine.NewSnapshot is not an exception because it's still true that it "will not see future mutations". What's special about it, if I understand correctly, is that it also won't see some "past" mutations. Consider clarifying the language some more; I recognize that the struggle is real.
pkg/storage/pebble.go, line 782 at r1 (raw file):
} func (p *Pebble) PinEngineStateForIterators() error {
ditto here and below about commenting what interface this method comes from.
pkg/storage/pebble.go, line 1250 at r1 (raw file):
// the fact that all pebbleIterators created here are marked as reusable, // which causes pebbleIterator.Close to not close iter. iter will be closed // when pebbleReadOnly.Close is called. The caller can change the above
nit: I found the addition to this comment a bit confusing. The paragraph generally talks about how iterator creation clones an existing one - so it mostly talks about all iterators but the first one. And now you say that PinEngineStateForIterators can change something about "the above", when in fact the only thing it changes is something about the first one. Consider clarifying, and perhaps giving the new note it's own paragraph.
There was a problem hiding this comment.
TFTR!
You don't seem to actually have made the change to switch reads to use the new pinning. Why not? It'd be good to do it in this PR, I'd say.
There is no benefit derived yet from pinning. It's best if the PR that actually utilizes this for some benefit, like releasing latches early, does that.
Reviewable status:
pkg/kv/kvserver/spanset/batch.go, line 486 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
it's a wide-spread standard in the codebase to put a comment saying
PingEngine... is part of the storage.Readerinterace`. I see that it's not done around here, but consider starting to do it; I find these comments quite useful.
Oversight on my part. Luckily you and lint caught it.
This file is inconsistent with such comments (unlike the storage package), so I've not bothered with the existing methods, though I did change the engine package name to storage.
pkg/storage/engine.go, line 367 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
does this comment need updating?
Good catch. Done
pkg/storage/engine.go, line 447 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
This concept of a "timestamp hint" appears now 3 times in the comments here, but it's not explained anywhere that I can see. Should it be?
I've added a parenthetical reference to IterOptions in all these places.
pkg/storage/engine.go, line 447 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
what does it mean for "this" to "not apply" to some iterators? Does that mean that those iterators will still be "inconsistent" even when
ConsistentIteratorsreturns true? Consider expanding the comment.
Correct. Added a comment.
pkg/storage/engine.go, line 453 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
will not see future mutations. The exception is the implementation thatIt sounds to me like, technically, the
Engine.NewSnapshotis not an exception because it's still true that it "will not see future mutations". What's special about it, if I understand correctly, is that it also won't see some "past" mutations. Consider clarifying the language some more; I recognize that the struggle is real.
Done
pkg/storage/pebble.go, line 782 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
ditto here and below about commenting what interface this method comes from.
Done
pkg/storage/pebble.go, line 1250 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit: I found the addition to this comment a bit confusing. The paragraph generally talks about how iterator creation clones an existing one - so it mostly talks about all iterators but the first one. And now you say that
PinEngineStateForIteratorscan change something about "the above", when in fact the only thing it changes is something about the first one. Consider clarifying, and perhaps giving the new note it's own paragraph.
Done. I've mentioned this in the first sentence itself since that is where it belongs.
There was a problem hiding this comment.
There is no benefit derived yet from pinning. It's best if the PR that actually utilizes this for some benefit, like releasing latches early, does that.
I understand, but I still think it'd be a good idea to just do it in this patch. These pinned iterators come up as a requirement so often, and they come up in discussions where a million other pre-reqs are also required. There's uncertainty about how things work, and it also seems like there's too many mountains to move to get anywhere, so usually we get scared and lazy. Unambiguously saying that requests run on top of snapshot captured at a easily-determined time would be a great conceptual simplification, at a time when we're starving for conceptual simplifications. Consider this social engineering factor in the decision. But obvi as you wish.
Reviewable status:
This is relevant for read evaluation cases where we want to release latches before evaluation. The new Reader.PinEngineStateForIterators method would be called before releasing latches. This pinning does not apply to iterators with timestamp hints, similar to how ConsistentIterators does not apply to them. So this does not help ExportRequest evaluation for incremental backups. To address this we would want a guarantee from the caller that the timestamp hints will not change for the lifetime of the Reader that is making a promise of ConsistentIterators. Informs cockroachdb#55461,cockroachdb#66485 Release note: None
There was a problem hiding this comment.
Reviewed 1 of 5 files at r1, 3 of 4 files at r2.
Reviewable status:
There was a problem hiding this comment.
@andreimatei I sympathize with what you are saying but I'm wary of adding something that has no value in isolation, and I also don't wish to run a bunch of regression tests to confirm that this doesn't regress some workloads. I assume what is happening is that everyones time is fragmented because of other reasons. What we really need is someone to have the bandwidth to write a (short) design doc that a broader set of folks can stare at to check that there isn't a correctness flaw and then go for it. I was assuming you were picking this up, and this would unblock some part of your work.
Reviewable status:
|
bors r+ |
|
Build succeeded: |
This is relevant for read evaluation cases where we want to release
latches before evaluation. The new Reader.PinEngineStateForIterators
method would be called before releasing latches.
This pinning does not apply to iterators with timestamp hints,
similar to how ConsistentIterators does not apply to them. So
this does not help ExportRequest evaluation for incremental backups.
To address this we would want a guarantee from the caller that the
timestamp hints will not change for the lifetime of the Reader that
is making a promise of ConsistentIterators.
Informs #55461,#66485
Release note: None