From 7bea05ddaab4cd5d52e5d22cc34784245c147af1 Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Thu, 9 Nov 2023 16:46:18 -0500 Subject: [PATCH] kv: use correct strength for {Get,Scan,ReverseScan} for SKIP LOCKED TODO: add some tests for this. TODO: polish. --- pkg/kv/kvserver/batcheval/cmd_get.go | 7 ++++- pkg/kv/kvserver/batcheval/cmd_reverse_scan.go | 7 ++++- pkg/kv/kvserver/batcheval/cmd_scan.go | 7 ++++- pkg/kv/kvserver/batcheval/lock.go | 30 +++++++++++++++++++ pkg/storage/mvcc.go | 8 +---- pkg/storage/pebble_mvcc_scanner.go | 7 +---- 6 files changed, 50 insertions(+), 16 deletions(-) diff --git a/pkg/kv/kvserver/batcheval/cmd_get.go b/pkg/kv/kvserver/batcheval/cmd_get.go index 1791f80746c6..055c275c4041 100644 --- a/pkg/kv/kvserver/batcheval/cmd_get.go +++ b/pkg/kv/kvserver/batcheval/cmd_get.go @@ -33,6 +33,11 @@ func Get( h := cArgs.Header reply := resp.(*kvpb.GetResponse) + var lockTableForSkipLocked storage.LockTableView + if h.WaitPolicy == lock.WaitPolicy_SkipLocked { + lockTableForSkipLocked = newRequestBoundLockTableView(cArgs.Concurrency, args.KeyLockingStrength) + } + getRes, err := storage.MVCCGet(ctx, readWriter, args.Key, h.Timestamp, storage.MVCCGetOptions{ Inconsistent: h.ReadConsistency != kvpb.CONSISTENT, SkipLocked: h.WaitPolicy == lock.WaitPolicy_SkipLocked, @@ -41,7 +46,7 @@ func Get( ScanStats: cArgs.ScanStats, Uncertainty: cArgs.Uncertainty, MemoryAccount: cArgs.EvalCtx.GetResponseMemoryAccount(), - LockTable: cArgs.Concurrency, + LockTable: lockTableForSkipLocked, DontInterleaveIntents: cArgs.DontInterleaveIntents, MaxKeys: cArgs.Header.MaxSpanRequestKeys, TargetBytes: cArgs.Header.TargetBytes, diff --git a/pkg/kv/kvserver/batcheval/cmd_reverse_scan.go b/pkg/kv/kvserver/batcheval/cmd_reverse_scan.go index 45a734814b2c..6a62103aba30 100644 --- a/pkg/kv/kvserver/batcheval/cmd_reverse_scan.go +++ b/pkg/kv/kvserver/batcheval/cmd_reverse_scan.go @@ -35,6 +35,11 @@ func ReverseScan( h := cArgs.Header reply := resp.(*kvpb.ReverseScanResponse) + var lockTableForSkipLocked storage.LockTableView + if h.WaitPolicy == lock.WaitPolicy_SkipLocked { + lockTableForSkipLocked = newRequestBoundLockTableView(cArgs.Concurrency, args.KeyLockingStrength) + } + var res result.Result var scanRes storage.MVCCScanResult var err error @@ -53,7 +58,7 @@ func ReverseScan( FailOnMoreRecent: args.KeyLockingStrength != lock.None, Reverse: true, MemoryAccount: cArgs.EvalCtx.GetResponseMemoryAccount(), - LockTable: cArgs.Concurrency, + LockTable: lockTableForSkipLocked, DontInterleaveIntents: cArgs.DontInterleaveIntents, } diff --git a/pkg/kv/kvserver/batcheval/cmd_scan.go b/pkg/kv/kvserver/batcheval/cmd_scan.go index 7d251655a541..5af386064f5c 100644 --- a/pkg/kv/kvserver/batcheval/cmd_scan.go +++ b/pkg/kv/kvserver/batcheval/cmd_scan.go @@ -35,6 +35,11 @@ func Scan( h := cArgs.Header reply := resp.(*kvpb.ScanResponse) + var lockTableForSkipLocked storage.LockTableView + if h.WaitPolicy == lock.WaitPolicy_SkipLocked { + lockTableForSkipLocked = newRequestBoundLockTableView(cArgs.Concurrency, args.KeyLockingStrength) + } + var res result.Result var scanRes storage.MVCCScanResult var err error @@ -53,7 +58,7 @@ func Scan( FailOnMoreRecent: args.KeyLockingStrength != lock.None, Reverse: false, MemoryAccount: cArgs.EvalCtx.GetResponseMemoryAccount(), - LockTable: cArgs.Concurrency, + LockTable: lockTableForSkipLocked, DontInterleaveIntents: cArgs.DontInterleaveIntents, } diff --git a/pkg/kv/kvserver/batcheval/lock.go b/pkg/kv/kvserver/batcheval/lock.go index 3e626a62ca8c..a722216cafa8 100644 --- a/pkg/kv/kvserver/batcheval/lock.go +++ b/pkg/kv/kvserver/batcheval/lock.go @@ -224,3 +224,33 @@ func copyKey(k roachpb.Key) roachpb.Key { copy(k2, k) return k2 } + +// txnBoundLockTableView is a transaction-bound view into an in-memory +// collections of key-level locks. +type txnBoundLockTableView interface { + IsKeyLockedByConflictingTxn( + roachpb.Key, lock.Strength, + ) (bool, *enginepb.TxnMeta, error) +} + +// requestBoundLockTableView combines a txnBoundLockTableView with the lock +// strength that an individual request is attempting to acquire. +type requestBoundLockTableView struct { + ltv txnBoundLockTableView + str lock.Strength +} + +// newRequestBoundLockTableView creates a new requestBoundLockTableView. +func newRequestBoundLockTableView( + ltv txnBoundLockTableView, str lock.Strength, +) *requestBoundLockTableView { + return &requestBoundLockTableView{ltv: ltv, str: str} +} + +// IsKeyLockedByConflictingTxn implements the storage.LockTableView interface. +func (ltv *requestBoundLockTableView) IsKeyLockedByConflictingTxn( + key roachpb.Key, +) (bool, *enginepb.TxnMeta, error) { + // TODO(nvanbenschoten): look for replicated lock conflicts. + return ltv.ltv.IsKeyLockedByConflictingTxn(key, ltv.str) +} diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index 96b9e5a61b93..e0b6af80fd96 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -1190,13 +1190,7 @@ type LockTableView interface { // // This method is used by requests in conjunction with the SkipLocked wait // policy to determine which keys they should skip over during evaluation. - // - // If the supplied lock strength is locking (!= lock.None), then any queued - // locking requests that came before the lockTableGuard will also be checked - // for conflicts. This helps prevent a stream of locking SKIP LOCKED requests - // from starving out regular locking requests. In such cases, true is - // returned, but so is nil. - IsKeyLockedByConflictingTxn(roachpb.Key, lock.Strength) (bool, *enginepb.TxnMeta, error) + IsKeyLockedByConflictingTxn(roachpb.Key) (bool, *enginepb.TxnMeta, error) } // MVCCGetOptions bundles options for the MVCCGet family of functions. diff --git a/pkg/storage/pebble_mvcc_scanner.go b/pkg/storage/pebble_mvcc_scanner.go index feb5f3250dc8..2aca6e6a6f2b 100644 --- a/pkg/storage/pebble_mvcc_scanner.go +++ b/pkg/storage/pebble_mvcc_scanner.go @@ -19,7 +19,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv/kvpb" - "github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/uncertainty" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" @@ -1805,11 +1804,7 @@ func (p *pebbleMVCCScanner) isKeyLockedByConflictingTxn( p.err = err return false, false } - strength := lock.None - if p.failOnMoreRecent { - strength = lock.Exclusive - } - ok, txn, err := p.lockTable.IsKeyLockedByConflictingTxn(key, strength) + ok, txn, err := p.lockTable.IsKeyLockedByConflictingTxn(key) if err != nil { p.err = err return false, false