kv: establish new read snapshot on statement/batch boundaries under RC#104362
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Jun 14, 2023
Merged
Conversation
Member
13deade to
c5aed0d
Compare
arulajmani
approved these changes
Jun 12, 2023
Collaborator
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 4 of 5 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvclient/kvcoord/txn_test.go line 390 at r3 (raw file):
defer log.Scope(t).Close(t) run := func(isoLevel isolation.Level, mode kv.SteppingMode, step bool, exp bool) {
nit: consider renaming exp to something more descriptive.
Closes cockroachdb#100133. This commit teaches Read Committed transactions to establish a new external "read snapshot" on each SQL statement or KV batch boundary. The per-statement/batch read snapshot logic is integrated into the transaction `Stepping` infrastructure in an analogous manner to how the transaction's internal `readSeq` is advanced. For transactions with stepping enabled (e.g. SQL transactions), the `Step` API now advances the transaction's external read snapshot (i.e. `ReadTimestamp`) to a timestamp captured from the local HLC clock. This ensures that subsequent read-only operations observe the writes of other transactions that were committed before the time the statement began. For transactions with stepping disabled (e.g. raw KV transactions), each batch implicitly advances the read snapshot. This ensures that the batch observes the writes of other transactions that were committed before the batch was issued. This read snapshot will be at least as recent as the previous read snapshot, and will typically be more recent (i.e. it will never regress). Consistency with prior reads in the transaction is not maintained, so reads of previously read keys may not be "repeatable" and may observe "phantoms". On the other hand, by not maintaining consistency between read snapshots, isolation-related retries (write-write conflicts) and consistency-related retries (uncertainty errors) have a higher chance of being refreshed away (client-side or server-side) without need for client intervention (i.e. without requiring a statement-level retry). This benefit can be seen in `TestTxnCoordSenderRetries`. As described in the Read Committed RFC, the transaction's uncertainty interval is not reset when establishing a new read snapshot. See section "Read Uncertainty Intervals" of the RFC for the rationale behind this decision. Release note: None
c5aed0d to
f8e5a8a
Compare
nvb
commented
Jun 14, 2023
Contributor
Author
nvb
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani)
pkg/kv/kvclient/kvcoord/txn_test.go line 390 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: consider renaming exp to something more descriptive.
Done.
Contributor
|
Build succeeded: |
This file contains hidden or 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
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.
Closes #100133.
This commit teaches Read Committed transactions to establish a new external "read snapshot" on each SQL statement or KV batch boundary.
The per-statement/batch read snapshot logic is integrated into the transaction
Steppinginfrastructure in an analogous manner to how the transaction's internalreadSeqis advanced. For transactions with stepping enabled (e.g. SQL transactions), theStepAPI now advances the transaction's external read snapshot (i.e.ReadTimestamp) to a timestamp captured from the local HLC clock. This ensures that subsequent read-only operations observe the writes of other transactions that were committed before the time the statement began. For transactions with stepping disabled (e.g. raw KV transactions), each batch implicitly advances the read snapshot. This ensures that the batch observes the writes of other transactions that were committed before the batch was issued.This read snapshot will be at least as recent as the previous read snapshot, and will typically be more recent (i.e. it will never regress). Consistency with prior reads in the transaction is not maintained, so reads of previously read keys may not be "repeatable" and may observe "phantoms". On the other hand, by not maintaining consistency between read snapshots, isolation-related retries (write-write conflicts) and consistency-related retries (uncertainty errors) have a higher chance of being refreshed away (client-side or server-side) without the need for client intervention (i.e. without requiring a statement-level retry). This benefit can be seen in
TestTxnCoordSenderRetries.As described in the Read Committed RFC, the transaction's uncertainty interval is not reset when establishing a new read snapshot. See section "Read Uncertainty Intervals" of the RFC for the rationale behind this decision.
Release note: None