From 6105313cca07ce8a64292326eb2eee6e1b784bde Mon Sep 17 00:00:00 2001 From: Pavel Kalinnikov Date: Thu, 20 Nov 2025 15:59:29 +0000 Subject: [PATCH] kvserver: disable latch assertions further down This commit moves disabling of the latch assertions from the latch declaration phase to the body of the corresponding commands. This serves two purposes: 1. Disabling of the assertions is now more targeted. It is per command rather than per batch. 2. The disabling behaviour is ephemeral, so we can remove the bool in SpanSet that doesn't belong there. It's a data structure that should have no opinion on whether the checks are enabled. Epic: none Release note: none --- pkg/kv/kvserver/batcheval/cmd_export.go | 11 ++++++----- pkg/kv/kvserver/batcheval/cmd_gc.go | 4 +++- .../kvserver/batcheval/cmd_lease_request.go | 8 +++++--- .../kvserver/batcheval/cmd_recompute_stats.go | 7 ++++--- pkg/kv/kvserver/spanset/spanset.go | 19 +------------------ 5 files changed, 19 insertions(+), 30 deletions(-) diff --git a/pkg/kv/kvserver/batcheval/cmd_export.go b/pkg/kv/kvserver/batcheval/cmd_export.go index 2fe89b857a2b..5bfc5c6fc593 100644 --- a/pkg/kv/kvserver/batcheval/cmd_export.go +++ b/pkg/kv/kvserver/batcheval/cmd_export.go @@ -76,11 +76,6 @@ func declareKeysExport( return err } latchSpans.AddNonMVCC(spanset.SpanReadOnly, roachpb.Span{Key: keys.RangeGCThresholdKey(header.RangeID)}) - // Export requests will usually not hold latches during their evaluation. - // - // See call to `AssertAllowed()` in GetGCThreshold() to understand why we need - // to disable these assertions for export requests. - latchSpans.DisableUndeclaredAccessAssertions() return nil } @@ -96,6 +91,12 @@ func evalExport( ctx, evalExportSpan := tracing.ChildSpan(ctx, "evalExport") defer evalExportSpan.Finish() + // Export requests will usually not hold latches during their evaluation. + // + // See call to `AssertAllowed()` in GetGCThreshold() to understand why we need + // to disable these assertions for export requests. + reader = spanset.DisableReaderAssertions(reader) + // Table's marked to be excluded from backup are expected to be configured // with a short GC TTL. Additionally, backup excludes such table's from being // protected from GC when writing ProtectedTimestamp records. The diff --git a/pkg/kv/kvserver/batcheval/cmd_gc.go b/pkg/kv/kvserver/batcheval/cmd_gc.go index 80cb1a4a75bb..f1a054a00d89 100644 --- a/pkg/kv/kvserver/batcheval/cmd_gc.go +++ b/pkg/kv/kvserver/batcheval/cmd_gc.go @@ -106,7 +106,6 @@ func declareKeysGC( latchSpans.AddNonMVCC(spanset.SpanReadOnly, roachpb.Span{Key: keys.RangeDescriptorKey(rs.GetStartKey())}) // Needed for updating optional GC hint. latchSpans.AddNonMVCC(spanset.SpanReadWrite, roachpb.Span{Key: keys.RangeGCHintKey(rs.GetRangeID())}) - latchSpans.DisableUndeclaredAccessAssertions() return nil } @@ -139,6 +138,9 @@ func GC( args := cArgs.Args.(*kvpb.GCRequest) h := cArgs.Header + // TODO(pav-kv): find out why and comment. + readWriter = spanset.DisableReadWriterAssertions(readWriter) + // We do not allow GC requests to bump the GC threshold at the same time that // they GC individual keys. This is because performing both of these actions // at the same time could lead to a race where a read request is allowed to diff --git a/pkg/kv/kvserver/batcheval/cmd_lease_request.go b/pkg/kv/kvserver/batcheval/cmd_lease_request.go index 0c18aecd1cc8..209ef0eb9350 100644 --- a/pkg/kv/kvserver/batcheval/cmd_lease_request.go +++ b/pkg/kv/kvserver/batcheval/cmd_lease_request.go @@ -23,10 +23,10 @@ func init() { } func declareKeysRequestLease( - rs ImmutableRangeState, + _ ImmutableRangeState, _ *kvpb.Header, _ kvpb.Request, - latchSpans *spanset.SpanSet, + _ *spanset.SpanSet, _ *lockspanset.LockSpanSet, _ time.Duration, ) error { @@ -34,7 +34,6 @@ func declareKeysRequestLease( // acquiring latches would not help synchronize with other requests. As // such, the request does not declare latches. See also // concurrency.shouldIgnoreLatches(). - latchSpans.DisableUndeclaredAccessAssertions() return nil } @@ -54,6 +53,9 @@ func RequestLease( prevLease := args.PrevLease newLease := args.Lease + // RequestLease does not hold latches by design. + readWriter = spanset.DisableReadWriterAssertions(readWriter) + // If this check is removed at some point, the filtering of learners on the // sending side would have to be removed as well. // TODO(nvanbenschoten): move this into leases.Verify. diff --git a/pkg/kv/kvserver/batcheval/cmd_recompute_stats.go b/pkg/kv/kvserver/batcheval/cmd_recompute_stats.go index f532fcd08d3a..12619f95df03 100644 --- a/pkg/kv/kvserver/batcheval/cmd_recompute_stats.go +++ b/pkg/kv/kvserver/batcheval/cmd_recompute_stats.go @@ -51,8 +51,6 @@ func declareKeysRecomputeStats( rdKey := keys.RangeDescriptorKey(rs.GetStartKey()) latchSpans.AddNonMVCC(spanset.SpanReadOnly, roachpb.Span{Key: rdKey}) latchSpans.AddNonMVCC(spanset.SpanReadWrite, roachpb.Span{Key: keys.TransactionKey(rdKey, uuid.Nil)}) - // Disable the assertions which check that all reads were previously declared. - latchSpans.DisableUndeclaredAccessAssertions() return nil } @@ -72,9 +70,12 @@ func RecomputeStats( return result.Result{}, RecomputeStatsMismatchError } dryRun := args.DryRun - args = nil // avoid accidental use below + // Disable the assertions which check that all reads were previously declared. + // TODO(pav-kv): find out why we can do that. + reader = spanset.DisableReaderAssertions(reader) + actualMS, err := rditer.ComputeStatsForRange(ctx, desc, reader, cArgs.Header.Timestamp.WallTime) if err != nil { return result.Result{}, err diff --git a/pkg/kv/kvserver/spanset/spanset.go b/pkg/kv/kvserver/spanset/spanset.go index f9df171dd2ab..5cc909f6891c 100644 --- a/pkg/kv/kvserver/spanset/spanset.go +++ b/pkg/kv/kvserver/spanset/spanset.go @@ -83,8 +83,7 @@ type Span struct { // The Span slice for a particular access and scope contains non-overlapping // spans in increasing key order after calls to SortAndDedup. type SpanSet struct { - spans [NumSpanAccess][NumSpanScope][]Span - allowUndeclared bool + spans [NumSpanAccess][NumSpanScope][]Span } var spanSetPool = sync.Pool{ @@ -113,7 +112,6 @@ func (s *SpanSet) Release() { s.spans[sa][ss] = recycle } } - s.allowUndeclared = false spanSetPool.Put(s) } @@ -155,7 +153,6 @@ func (s *SpanSet) Copy() *SpanSet { n.spans[sa][ss] = append(n.spans[sa][ss], s.spans[sa][ss]...) } } - n.allowUndeclared = s.allowUndeclared return n } @@ -208,7 +205,6 @@ func (s *SpanSet) Merge(s2 *SpanSet) { s.spans[sa][ss] = append(s.spans[sa][ss], s2.spans[sa][ss]...) } } - s.allowUndeclared = s2.allowUndeclared s.SortAndDedup() } @@ -331,12 +327,6 @@ func (s *SpanSet) CheckAllowedAt( func (s *SpanSet) checkAllowed( access SpanAccess, span roachpb.Span, check func(SpanAccess, Span) bool, ) error { - if s.allowUndeclared { - // If the request has specified that undeclared spans are allowed, do - // nothing. - return nil - } - scope := SpanGlobal if (span.Key != nil && keys.IsLocal(span.Key)) || (span.EndKey != nil && keys.IsLocal(span.EndKey)) { @@ -389,10 +379,3 @@ func (s *SpanSet) Validate() error { return nil } - -// DisableUndeclaredAccessAssertions disables the assertions that prevent -// undeclared access to spans. This is generally set by requests that rely on -// other forms of synchronization for correctness (e.g. GCRequest). -func (s *SpanSet) DisableUndeclaredAccessAssertions() { - s.allowUndeclared = true -}