storage: Don't advance keys in MVCCGet with Pebble#48239
storage: Don't advance keys in MVCCGet with Pebble#48239craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
petermattis
left a comment
There was a problem hiding this comment.
Looks good modulo a few small comments.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @itsbilal, @jbowens, @petermattis, and @tbg)
pkg/storage/mvcc.go, line 794 at r1 (raw file):
mvccScanner.init(opts.Txn) mvccScanner.get()
Perhaps we should set isGet = true in .get() and isGet = false in .scan().
pkg/storage/pebble_mvcc_scanner.go, line 220 at r1 (raw file):
// unmarshalled already. Returns true if a value was read and added to the // result set. func (p *pebbleMVCCScanner) getFromIntentHistory() (found bool, value []byte) {
It would be slight more idiomatic for this to be value []byte, found bool. That would match up with the multi-value version of map lookup and with type assertions where the second parameter is the boolean.
pkg/storage/pebble_mvcc_scanner.go, line 244 at r1 (raw file):
return false, nil } intent := p.meta.IntentHistory[upIdx-1]
This should probably be intent := &p.meta.IntentHistory[upIdx-1].
Previously, when doing an MVCCGet with the Pebble MVCC scanner, we would advance keys after getting (or not getting) the value we're looking for. In some cases this could result in a seek, such as if there are too many revisions of that MVCC key. Seeks are costly and at best avoided when it's ultimatey unnecessary. Another side effect of doing this seek was that it would trip an assertion in spanSet.Iterator that checked if all reads were happening on allowed key spans. This caused TestDumpData to fail on Pebble (but not on RocksDB since the RocksDB MVCC scanner is specialized in C++ and doesn't flow through the same assertion for seeks). There's one additional change, stemming from a follow-up conversation in cockroachdb#48160, around unifying logic for terminating an MVCC scan. The getFromIntentHistory() case also calls into addAndAdvance() now. Fixes cockroachdb#48147. Release note: None.
2b8cf05 to
d0da8ef
Compare
itsbilal
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/storage/mvcc.go, line 794 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Perhaps we should set
isGet = truein.get()andisGet = falsein.scan().
Done.
pkg/storage/pebble_mvcc_scanner.go, line 220 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
It would be slight more idiomatic for this to be
value []byte, found bool. That would match up with the multi-value version of map lookup and with type assertions where the second parameter is the boolean.
Done.
pkg/storage/pebble_mvcc_scanner.go, line 244 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
This should probably be
intent := &p.meta.IntentHistory[upIdx-1].
Done.
petermattis
left a comment
There was a problem hiding this comment.
Reviewable status:
|
❌ The GitHub CI (Cockroach) build has failed on d0da8ef8. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
|
TFTR! bors r+ |
Build failed (retrying...) |
Build succeeded |
Previously, when doing an MVCCGet with the Pebble MVCC scanner,
we would advance keys after getting (or not getting) the value
we're looking for. In some cases this could result in a seek,
such as if there are too many revisions of that MVCC key. Seeks
are costly and at best avoided when it's ultimatey unnecessary.
Another side effect of doing this seek was that it would trip
an assertion in spanSet.Iterator that checked if all reads
were happening on allowed key spans. This caused TestDumpData
to fail on Pebble (but not on RocksDB since the RocksDB MVCC
scanner is specialized in C++ and doesn't flow through the same
assertion for seeks).
There's one additional change, stemming from a follow-up conversation
in #48160, around unifying logic for terminating an MVCC scan.
The getFromIntentHistory() case also calls into addAndAdvance()
now.
Fixes #48147.
Release note: None.