release-20.1: engine: set the MVCC timestamp on reads due to historical intents #54942
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
ajwerner
force-pushed
the
backport20.1-52358
branch
from
6bc9214 to
6ea9ede
Compare
|
@itsbilal the change differs somewhat from the change made in 20.2 given the different structure of the pebble code. Here is more resembles the rocksdb code. |
itsbilal
approved these changes
Sep 29, 2020
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)
pkg/storage/pebble_mvcc_scanner.go, line 253 at r1 (raw file):
// without an MVCC timestamp. if key.Timestamp.IsEmpty() { key.Timestamp = hlc.Timestamp{
Nit: This can be just key.Timestamp = p.meta.Timestamp
Before this commit, we'd return a zero-value MVCC timestamp when reading an intent from the intent history. This was problematic because elsewhere in the code we assume that we will always get a non-zero MVCC timestamp when a read returns a value. This is especially bizarre given that a read of the latest intent will return its write timestamp. The semantics here are such that we'll return the timestamp of the MVCC metadata for the row. I think this is the most reasonable thing to do as that timestamp ought to reflect the timestamp we return when we return the current intent and furthermore is the only timestamp we really have around. We could return the transactions current read or write timestamp but those both seem like worse choices. It's also worth noting that in the case where we need to read a non-zero value, we don't really care what that value is and the fact that we are reading this intent itself is somewhat suspect. That being said, I still think we should make this change in addition to any change made to prevent the code referenced in cockroachdb#49266 from needing it. Fixes cockroachdb#49266. Informs cockroachdb#50102. Release note: None
ajwerner
force-pushed
the
backport20.1-52358
branch
from
6ea9ede to
deb3ced
Compare
ajwerner
commented
Sep 29, 2020
There was a problem hiding this comment.
Reviewable status:
pkg/storage/pebble_mvcc_scanner.go, line 253 at r1 (raw file):
Previously, itsbilal (Bilal Akhtar) wrote…
Nit: This can be just key.Timestamp = p.meta.Timestamp
Done. Needed a cast.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 1/1 commits from #52358.
/cc @cockroachdb/release
Fixes #54462.
Before this commit, we'd return a zero-value MVCC timestamp when reading an
intent from the intent history. This was problematic because elsewhere in the
code we assume that we will always get a non-zero MVCC timestamp when a read
returns a value. This is especially bizarre given that a read of the latest
intent will return its write timestamp.
The semantics here are such that we'll return the timestamp of the MVCC
metadata for the row. I think this is the most reasonable thing to do as
that timestamp ought to reflect the timestamp we return when we return the
current intent and furthermore is the only timestamp we really have around.
We could return the transactions current read or write timestamp but those
both seem like worse choices.
It's also worth noting that in the case where we need to read a non-zero
value, we don't really care what that value is and the fact that we are
reading this intent itself is somewhat suspect. That being said, I still
think we should make this change in addition to any change made to prevent
the code referenced in #49266 from needing it.
Fixes #49266.
Informs #50102.
Release note: None