From 444e927901f92cfa5f5838536add87c49b836687 Mon Sep 17 00:00:00 2001 From: sumeerbhola Date: Tue, 17 Oct 2023 20:52:26 -0400 Subject: [PATCH] storage: restore optimization for inline puts/deletes We had an optimization that avoided constructing an iterator over the locktable key space when doing inline puts/deletes. This was lost in https://github.com/cockroachdb/cockroach/commit/ac14ebb197ecc596449a287aadba918176e47ca7 that constructed a lockTableKeyScanner even for inline puts/deletes. The optimization is restored here to fix the regression discussed in https://github.com/cockroachdb/cockroach/issues/111919#issuecomment-1764560921. Informs #111919 Epic: none Release note: None --- pkg/storage/mvcc.go | 114 ++++++++++++------ pkg/storage/testdata/mvcc_histories/inline | 32 +++++ .../testdata/mvcc_histories/replicated_locks | 2 +- 3 files changed, 109 insertions(+), 39 deletions(-) diff --git a/pkg/storage/mvcc.go b/pkg/storage/mvcc.go index 3473fdf09e5f..6e45d98de610 100644 --- a/pkg/storage/mvcc.go +++ b/pkg/storage/mvcc.go @@ -1837,11 +1837,14 @@ func MVCCPut( } defer iter.Close() - ltScanner, err = newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) - if err != nil { - return err + inlinePut := timestamp.IsEmpty() + if !inlinePut { + ltScanner, err = newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) + if err != nil { + return err + } + defer ltScanner.close() } - defer ltScanner.close() } return mvccPutUsingIter(ctx, rw, iter, ltScanner, key, timestamp, value, nil, opts) } @@ -1894,12 +1897,15 @@ func MVCCDelete( } defer iter.Close() - ltScanner, err := newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) - if err != nil { - return false, err + inlineDelete := timestamp.IsEmpty() + var ltScanner *lockTableKeyScanner + if !inlineDelete { + ltScanner, err = newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) + if err != nil { + return false, err + } + defer ltScanner.close() } - defer ltScanner.close() - buf := newPutBuffer() defer buf.release() @@ -2146,8 +2152,15 @@ func mvccPutInternal( } putIsBlind := iter == nil - if putIsBlind != (ltScanner == nil) { - return false, errors.Errorf("iter and ltScanner must both be nil or both be non-nil") + putIsInline := timestamp.IsEmpty() + if (putIsBlind || putIsInline) != (ltScanner == nil) { + if ltScanner == nil { + return false, errors.Errorf( + "ltScanner must be non-nil for putIsBlind %t, putIsInline %t", putIsBlind, putIsInline) + } else { + return false, errors.Errorf( + "ltScanner must be nil for putIsBlind %t, putIsInline %t", putIsBlind, putIsInline) + } } metaKey := MakeMVCCMetadataKey(key) @@ -2166,7 +2179,19 @@ func mvccPutInternal( return false, err } meta = &buf.meta + // Verify we're not mixing inline and non-inline values. + if ok && putIsInline != meta.IsInline() { + return false, errors.Errorf("%q: put is inline=%t, but existing value is inline=%t", + metaKey, putIsInline, meta.IsInline()) + } if ok && !meta.IsInline() { + // INVARIANTS: + // !putIsBlind + // !meta.IsInline() + // !meta.IsInline() => !putIsInline (due to previous if-block) + // !putIsInline && !putIsBlind => ltScanner != nil (due to error check earlier in function) + // So we can use ltScanner safely. + // // If at least one version is found, scan the lock table for conflicting // locks and/or an intent on the key from different transactions. If any // such conflicts are found, the lock table scanner will return a @@ -2193,12 +2218,6 @@ func mvccPutInternal( } } - // Verify we're not mixing inline and non-inline values. - putIsInline := timestamp.IsEmpty() - if ok && putIsInline != meta.IsInline() { - return false, errors.Errorf("%q: put is inline=%t, but existing value is inline=%t", - metaKey, putIsInline, meta.IsInline()) - } // Handle inline put. No IntentHistory is required for inline writes as they // aren't allowed within transactions. MVCC range tombstones cannot exist // across them either. @@ -2662,11 +2681,15 @@ func MVCCIncrement( } defer iter.Close() - ltScanner, err := newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) - if err != nil { - return 0, err + inlineIncrement := timestamp.IsEmpty() + var ltScanner *lockTableKeyScanner + if !inlineIncrement { + ltScanner, err = newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) + if err != nil { + return 0, err + } + defer ltScanner.close() } - defer ltScanner.close() var int64Val int64 var newInt64Val int64 @@ -2749,12 +2772,15 @@ func MVCCConditionalPut( } defer iter.Close() - ltScanner, err := newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) - if err != nil { - return err + inlinePut := timestamp.IsEmpty() + var ltScanner *lockTableKeyScanner + if !inlinePut { + ltScanner, err = newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) + if err != nil { + return err + } + defer ltScanner.close() } - defer ltScanner.close() - return mvccConditionalPutUsingIter( ctx, rw, iter, ltScanner, key, timestamp, value, expVal, allowIfDoesNotExist, opts) } @@ -2840,11 +2866,15 @@ func MVCCInitPut( } defer iter.Close() - ltScanner, err := newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) - if err != nil { - return err + inlinePut := timestamp.IsEmpty() + var ltScanner *lockTableKeyScanner + if !inlinePut { + ltScanner, err = newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) + if err != nil { + return err + } + defer ltScanner.close() } - defer ltScanner.close() return mvccInitPutUsingIter(ctx, rw, iter, ltScanner, key, timestamp, value, failOnTombstones, opts) } @@ -3489,11 +3519,15 @@ func MVCCDeleteRange( } defer iter.Close() - ltScanner, err := newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) - if err != nil { - return nil, nil, 0, err + inlineDelete := timestamp.IsEmpty() + var ltScanner *lockTableKeyScanner + if !inlineDelete { + ltScanner, err = newLockTableKeyScanner(rw, opts.Txn, lock.Intent, opts.MaxLockConflicts) + if err != nil { + return nil, nil, 0, err + } + defer ltScanner.close() } - defer ltScanner.close() buf := newPutBuffer() defer buf.release() @@ -3664,11 +3698,15 @@ func MVCCPredicateDeleteRange( } defer pointTombstoneIter.Close() - ltScanner, err := newLockTableKeyScanner(rw, nil /* txn */, lock.Intent, maxLockConflicts) - if err != nil { - return nil, err + inlineDelete := endTime.IsEmpty() + var ltScanner *lockTableKeyScanner + if !inlineDelete { + ltScanner, err = newLockTableKeyScanner(rw, nil /* txn */, lock.Intent, maxLockConflicts) + if err != nil { + return nil, err + } + defer ltScanner.close() } - defer ltScanner.close() pointTombstoneBuf := newPutBuffer() defer pointTombstoneBuf.release() diff --git a/pkg/storage/testdata/mvcc_histories/inline b/pkg/storage/testdata/mvcc_histories/inline index d93168b233f4..43f07cdf9638 100644 --- a/pkg/storage/testdata/mvcc_histories/inline +++ b/pkg/storage/testdata/mvcc_histories/inline @@ -78,3 +78,35 @@ del_range k=i1 end=i4 del_range: "i1"-"i4" -> deleted 2 key(s) >> at end: + +# Non-inline put. +run ok +put k=i4 v=v4 ts=5,0 +---- +>> at end: +data: "i4"/5.000000000,0 -> /BYTES/v4 + +# Inline put for a key that is not inline. +run error +put k=i4 v=inline4 +---- +>> at end: +data: "i4"/5.000000000,0 -> /BYTES/v4 +error: (*withstack.withStack:) "i4"/0,0: put is inline=true, but existing value is inline=false + +# Inline put. +run ok +put k=i5 v=inline5 +---- +>> at end: +data: "i4"/5.000000000,0 -> /BYTES/v4 +meta: "i5"/0,0 -> txn={} ts=0,0 del=false klen=0 vlen=0 raw=/BYTES/inline5 mergeTs= txnDidNotUpdateMeta=false + +# Non-inline put for a key that is inline. +run error +put notxn k=i5 v=v5 ts=5,0 +---- +>> at end: +data: "i4"/5.000000000,0 -> /BYTES/v4 +meta: "i5"/0,0 -> txn={} ts=0,0 del=false klen=0 vlen=0 raw=/BYTES/inline5 mergeTs= txnDidNotUpdateMeta=false +error: (*withstack.withStack:) "i5"/0,0: put is inline=false, but existing value is inline=true diff --git a/pkg/storage/testdata/mvcc_histories/replicated_locks b/pkg/storage/testdata/mvcc_histories/replicated_locks index 74870779febf..42ab3fd6406e 100644 --- a/pkg/storage/testdata/mvcc_histories/replicated_locks +++ b/pkg/storage/testdata/mvcc_histories/replicated_locks @@ -433,7 +433,7 @@ data: "k4"/10.000000000,0 -> /BYTES/v4_prime error: (*kvpb.LockConflictError:) conflicting locks on "k1" run error -put notxn k=k1 v=v1 +put notxn ts=10,0 k=k1 v=v1 ---- >> at end: data: "k1"/5.000000000,0 -> /BYTES/v1