Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-24.2: kvnemesis: properly skip validation for rolled-back locking reads in weak-isolation transactions #129062

Merged
merged 1 commit into from
Aug 15, 2024
Merged

release-24.2: kvnemesis: properly skip validation for rolled-back locking reads in weak-isolation transactions #129062

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 28 additions & 14 deletions pkg/kv/kvnemesis/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,22 @@ type observedRead struct {
SkipLocked bool
Value roachpb.Value
ValidTimes disjointTimeSpans
// AlwaysValid indicates whether a read should be considered valid in the
// entire interval [hlc.MinTimestamp, hlc.MaxTimestamp].
//
// A lock acquired by a locking read is kept until commit time, unless it's
// rolled back by a savepoint; in that case, the lock is not released eagerly,
// but it's also not guaranteed to be held (if the transaction is pushed, the
// lock may be released). Serializable transactions refresh all reads at
// commit time, so their reads should be validated even if they were rolled
// back. Weaker-isolation transaction don't refresh their reads at commit
// time, so if a locking read from such a transaction is rolled back, we
// don't need to validate the read; it's valid at all times.
AlwaysValid bool
// DoNotObserveOnSavepointRollback indicates whether a read should be skipped
// from validation if it's rolled back by a savepoint. It's set to true for
// all locking reads from weak-isolation transactions.
DoNotObserveOnSavepointRollback bool
}

func (*observedRead) observedMarker() {}
Expand Down Expand Up @@ -437,6 +453,7 @@ func (v *validator) processOp(op Operation) {
// otherwise no lock would have been acquired on the non-existent key.
// Gets do not acquire gap locks.
observe = t.GuaranteedDurability && read.Value.IsPresent()
read.DoNotObserveOnSavepointRollback = true
default:
panic("unexpected")
}
Expand Down Expand Up @@ -753,9 +770,10 @@ func (v *validator) processOp(op Operation) {
if t.GuaranteedDurability {
for _, kv := range t.Result.Values {
read := &observedRead{
Key: kv.Key,
SkipLocked: t.SkipLocked,
Value: roachpb.Value{RawBytes: kv.Value},
Key: kv.Key,
SkipLocked: t.SkipLocked,
Value: roachpb.Value{RawBytes: kv.Value},
DoNotObserveOnSavepointRollback: true,
}
v.curObservations = append(v.curObservations, read)
}
Expand Down Expand Up @@ -1116,14 +1134,10 @@ func (v *validator) checkAtomicCommitted(
}
}
case *observedRead:
// If v.observationFilter == observeLocking, this must be a locking read
// from a weak-isolation transaction. If this locking read is rolled back
// by a savepoint, there is no guarantee that the lock will be present
// after the savepoint rollback. Since the read is not part of a
// serializatble transaction and is no longer locking, it's valid at all
// times. Alternatively, we can remove the read from txnObservations.
if rollbackSp != nil && v.observationFilter == observeLocking {
o.ValidTimes = disjointTimeSpans{{Start: hlc.MinTimestamp, End: hlc.MaxTimestamp}}
// If this read should not be observed on savepoint rollback, and there is
// a savepoint being rolled back, mark the read as valid at all times.
if rollbackSp != nil && o.DoNotObserveOnSavepointRollback {
o.AlwaysValid = true
}
case *observedSavepoint:
switch o.Type {
Expand Down Expand Up @@ -1197,9 +1211,9 @@ func (v *validator) checkAtomicCommitted(
}
}
case *observedRead:
// For rolled back locking reads from weak-isolation transactions, the
// ValidTimes are already set above.
if len(o.ValidTimes) == 0 {
if o.AlwaysValid {
o.ValidTimes = disjointTimeSpans{{Start: hlc.MinTimestamp, End: hlc.MaxTimestamp}}
} else {
o.ValidTimes = validReadTimes(batch, o.Key, o.Value.RawBytes, o.SkipLocked /* missingKeyValid */)
}
case *observedScan:
Expand Down