Merge pull request #113510 from sumeerbhola/backport23.2-112586

release-23.2: storage: restore optimization for inline puts/deletes

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()

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:
 <no data>
+
+# 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={<nil>} ts=0,0 del=false klen=0 vlen=0 raw=/BYTES/inline5 mergeTs=<nil> 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={<nil>} ts=0,0 del=false klen=0 vlen=0 raw=/BYTES/inline5 mergeTs=<nil> txnDidNotUpdateMeta=false
+error: (*withstack.withStack:) "i5"/0,0: put is inline=false, but existing value is inline=true

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