storage: require QueryTxn requests set a sufficient timestamp #36307
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 |
nvanbenschoten
force-pushed
the
nvanbenschoten/queryTxnTimestamp
branch
from
c5c9d1c to
fa99db6
Compare
tbg
approved these changes
Apr 17, 2019
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/storage/replica_command.go, line 377 at r1 (raw file):
// the new end key and then repeat the lookup inside the // transaction. // TODO(nvanbenschoten): Why can't we do this in the transaction?
You fixed this already on master, if I remember correctly.
This commit adds a check that QueryTxn requests set sufficiently high timestamps on their request header. This is in the spirit of cockroachdb#35297. Even though QueryTxn will never change the timestamp cache, I think there is a very rare hazard if it doesn't provide a timestamp at least as large as the transaction's timestamp in its batch header. If it doesn't do this, it seems possible that it could evaluate concurrently with a lease transfer (the lease transfer would have to start right after). The timestamp cache could then rotate pages a number of times until its low water mark is above the lease transfer timestamp (which is crazy as this would need to take at least 10s), and the QueryTxn could then consider a transaction ABORTED while evaluating because of the timestamp cache low water mark. The new leaseholder would have a lower timestamp cache low water mark and the transaction might still be able to create its transaction record. I don't think we would have ever seen this in the wild because of the 10s stall and the repeat timestamp cache rotations rotations, but this seems like a good change to make anyway. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/queryTxnTimestamp
branch
from
fa99db6 to
db1ee68
Compare
nvanbenschoten
commented
May 1, 2019
There was a problem hiding this comment.
sorry to let this sit for so long
No problem, I wasn't planning on merging until after the 19.1 release anyway. Thanks for the review!
bors r+
Reviewable status:
pkg/storage/replica_command.go, line 377 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
You fixed this already on
master, if I remember correctly.
Done.
craig bot
pushed a commit
that referenced
this pull request
May 1, 2019
36307: storage: require QueryTxn requests set a sufficient timestamp r=nvanbenschoten a=nvanbenschoten This commit adds a check that QueryTxn requests set sufficiently high timestamps on their request header. This is in the spirit of #35297. Even though QueryTxn will never change the timestamp cache, I think there is a very rare hazard if it doesn't provide a timestamp at least as large as the transaction's timestamp in its batch header. If it doesn't do this, it seems possible that it could evaluate concurrently with a lease transfer (the lease transfer would have to start right after). The timestamp cache could then rotate pages a number of times until its low water mark is above the lease transfer timestamp (which is crazy as this would need to take at least 10s), and the QueryTxn could then consider a transaction ABORTED while evaluating because of the timestamp cache low water mark. The new leaseholder would have a lower timestamp cache low water mark and the transaction might still be able to create its transaction record. I don't think we would have ever seen this in the wild because of the 10s stall and the repeat timestamp cache rotations necessary, but this seems like a good change to make anyway. Release note: None Co-authored-by: Nathan VanBenschoten <[email protected]>
Build succeeded |
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.
This commit adds a check that QueryTxn requests set sufficiently high
timestamps on their request header. This is in the spirit of #35297.
Even though QueryTxn will never change the timestamp cache, I think
there is a very rare hazard if it doesn't provide a timestamp at
least as large as the transaction's timestamp in its batch header.
If it doesn't do this, it seems possible that it could evaluate
concurrently with a lease transfer (the lease transfer would have
to start right after). The timestamp cache could then rotate pages
a number of times until its low water mark is above the lease
transfer timestamp (which is crazy as this would need to take at
least 10s), and the QueryTxn could then consider a transaction ABORTED
while evaluating because of the timestamp cache low water mark. The
new leaseholder would have a lower timestamp cache low water mark
and the transaction might still be able to create its transaction
record.
I don't think we would have ever seen this in the wild because of
the 10s stall and the repeat timestamp cache rotations necessary,
but this seems like a good change to make anyway.
Release note: None