kvserver,storage: handle ExpectExclusionSince during eval

Epic: none
Release note: None

Author: stevendanna

pkg/kv/kvserver/batcheval/cmd_get.go

@@ -15,6 +15,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/errors"
 )
 
 func init() {
@@ -42,24 +43,23 @@ func Get(
 	}
 
 	readTimestamp := h.Timestamp
-	moreRecentPolicy := storage.IgnoreMoreRecent
+	failOnMoreRecent := args.KeyLockingStrength != lock.None
+	expectExclusionSince := !args.ExpectExclusionSince.IsEmpty()
 
 	// If ExpectExclusionSince is set, use it as the read timestamp and set the
 	// MoreRecentPolicy to ExclusionViolationErrorOnMoreRecent to ensure that an
 	// exclusion violation error is returned if a write has occurred since the
 	// exclusion timestamp.
-	if !args.ExpectExclusionSince.IsEmpty() {
+	if expectExclusionSince {
 		readTimestamp = args.ExpectExclusionSince
-		moreRecentPolicy = storage.ExclusionViolationErrorOnMoreRecent
-	} else if args.KeyLockingStrength != lock.None {
-		moreRecentPolicy = storage.WriteTooOldErrorOnMoreRecent
+		failOnMoreRecent = true
 	}
 
 	getRes, err := storage.MVCCGet(ctx, readWriter, args.Key, readTimestamp, storage.MVCCGetOptions{
 		Inconsistent:          h.ReadConsistency != kvpb.CONSISTENT,
 		SkipLocked:            h.WaitPolicy == lock.WaitPolicy_SkipLocked,
 		Txn:                   h.Txn,
-		MoreRecentPolicy:      moreRecentPolicy,
+		FailOnMoreRecent:      failOnMoreRecent,
 		ScanStats:             cArgs.ScanStats,
 		Uncertainty:           cArgs.Uncertainty,
 		MemoryAccount:         cArgs.EvalCtx.GetResponseMemoryAccount(),
@@ -72,6 +72,17 @@ func Get(
 		ReturnRawMVCCValues:   args.ReturnRawMVCCValues,
 	})
 	if err != nil {
+		// If the user has set ExpectExclusionSince, transform any WriteTooOld error
+		// into an ExclusionViolationError.
+		if expectExclusionSince {
+			if wtoErr := (*kvpb.WriteTooOldError)(nil); errors.As(err, &wtoErr) {
+				err = kvpb.NewExclusionViolationError(
+					readTimestamp,
+					wtoErr.ActualTimestamp.Prev(),
+					args.Key,
+				)
+			}
+		}
 		return result.Result{}, err
 	}
 	reply.ResumeSpan = getRes.ResumeSpan

pkg/storage/mvcc.go

@@ -1230,28 +1230,13 @@ type LockTableView interface {
 	Close()
 }
 
-// MVCCGetMoreRecentPolicy determines how MVCCGet should handle more recent
-// versions of a key.
-type MVCCGetMoreRecentPolicy int
-
-const (
-	// IgnoreMoreRecent indicates that MVCCGet should ignore more recent versions of a key.
-	IgnoreMoreRecent MVCCGetMoreRecentPolicy = iota
-	// WriteTooOldErrorOnMoreRecent indicates that MVCCGet should return a WriteTooOldErrorOnMoreRecent
-	// if a more recent version of a key is found.
-	WriteTooOldErrorOnMoreRecent
-	// ExclusionViolationErrorOnMoreRecent indicates that MVCCGet should return an ExclusionLostError
-	// if a more recent version of a key is found.
-	ExclusionViolationErrorOnMoreRecent
-)
-
 // MVCCGetOptions bundles options for the MVCCGet family of functions.
 type MVCCGetOptions struct {
 	// See the documentation for MVCCGet for information on these parameters.
 	Inconsistent     bool
 	SkipLocked       bool
 	Tombstones       bool
-	MoreRecentPolicy MVCCGetMoreRecentPolicy
+	FailOnMoreRecent bool
 	Txn              *roachpb.Transaction
 	ScanStats        *kvpb.ScanStats
 	Uncertainty      uncertainty.Interval
@@ -1317,7 +1302,7 @@ func (opts *MVCCGetOptions) validate() error {
 	if opts.Inconsistent && opts.SkipLocked {
 		return errors.Errorf("cannot allow inconsistent reads with skip locked option")
 	}
-	if opts.Inconsistent && opts.MoreRecentPolicy != IgnoreMoreRecent {
+	if opts.Inconsistent && opts.FailOnMoreRecent {
 		return errors.Errorf("cannot allow inconsistent reads with fail on more recent option")
 	}
 	if opts.DontInterleaveIntents && opts.SkipLocked {
@@ -1587,7 +1572,7 @@ func mvccGet(
 		skipLocked:        opts.SkipLocked,
 		tombstones:        opts.Tombstones,
 		rawMVCCValues:     opts.ReturnRawMVCCValues,
-		moreRecentPolicy:  opts.MoreRecentPolicy,
+		failOnMoreRecent:  opts.FailOnMoreRecent,
 		keyBuf:            mvccScanner.keyBuf,
 		decodeMVCCHeaders: true,
 	}
@@ -4466,10 +4451,6 @@ func mvccScanInit(
 	if opts.MemoryAccount != nil {
 		memAccount = opts.MemoryAccount
 	}
-	moreRecentPolicy := IgnoreMoreRecent
-	if opts.FailOnMoreRecent {
-		moreRecentPolicy = WriteTooOldErrorOnMoreRecent
-	}
 	*mvccScanner = pebbleMVCCScanner{
 		parent:           iter,
 		memAccount:       memAccount,
@@ -4488,7 +4469,7 @@ func mvccScanInit(
 		inconsistent:     opts.Inconsistent,
 		skipLocked:       opts.SkipLocked,
 		tombstones:       opts.Tombstones,
-		moreRecentPolicy: moreRecentPolicy,
+		failOnMoreRecent: opts.FailOnMoreRecent,
 		keyBuf:           mvccScanner.keyBuf,
 		// NB: If the `results` argument passed to this function is a pointer to
 		// mvccScanner.alloc.pebbleResults, we don't want to overwrite any

pkg/storage/mvcc_history_test.go

@@ -94,7 +94,7 @@ var (
 // increment      [t=<name>] [ts=<int>[,<int>]] [localTs=<int>[,<int>]] [resolve [status=<txnstatus>]] [ambiguousReplay] [maxLockConflicts=<int>] [targetLockConflictBytes=<int>] k=<key> [inc=<val>]
 // put            [t=<name>] [ts=<int>[,<int>]] [localTs=<int>[,<int>]] [resolve [status=<txnstatus>]] [ambiguousReplay] [maxLockConflicts=<int>] k=<key> v=<string> [raw]
 // put_rangekey   ts=<int>[,<int>] [localTs=<int>[,<int>]] k=<key> end=<key> [syntheticBit]
-// put_blind_inline     k=<key> v=<string> [prev=<string>]
+// put_blind_inline	k=<key> v=<string> [prev=<string>]
 // get            [t=<name>] [ts=<int>[,<int>]]                         [resolve [status=<txnstatus>]] k=<key> [inconsistent] [skipLocked] [tombstones] [failOnMoreRecent] [localUncertaintyLimit=<int>[,<int>]] [globalUncertaintyLimit=<int>[,<int>]] [maxKeys=<int>] [targetBytes=<int>] [allowEmpty]
 // scan           [t=<name>] [ts=<int>[,<int>]]                         [resolve [status=<txnstatus>]] k=<key> [end=<key>] [inconsistent] [skipLocked] [tombstones] [reverse] [failOnMoreRecent] [localUncertaintyLimit=<int>[,<int>]] [globalUncertaintyLimit=<int>[,<int>]] [max=<max>] [targetbytes=<target>] [wholeRows[=<int>]] [allowEmpty]
 // export         [k=<key>] [end=<key>] [ts=<int>[,<int>]] [kTs=<int>[,<int>]] [startTs=<int>[,<int>]] [maxLockConflicts=<int>] [targetLockConflictBytes=<int>] [allRevisions] [targetSize=<int>] [maxSize=<int>] [stopMidKey] [fingerprint]
@@ -1396,24 +1396,8 @@ func cmdGet(e *evalCtx) error {
 		opts.Tombstones = true
 	}
 	if e.hasArg("failOnMoreRecent") {
-		opts.MoreRecentPolicy = storage.WriteTooOldErrorOnMoreRecent
-	}
-
-	if e.hasArg("moreRecentPolicy") {
-		var moreRecentPolicyStr string
-		e.scanArg("moreRecentPolicy", &moreRecentPolicyStr)
-		switch moreRecentPolicyStr {
-		case "ignoreMoreRecent":
-			opts.MoreRecentPolicy = storage.IgnoreMoreRecent
-		case "writeTooOldErrorOnMoreRecent":
-			opts.MoreRecentPolicy = storage.WriteTooOldErrorOnMoreRecent
-		case "exclusionViolationErrorOnMoreRecent":
-			opts.MoreRecentPolicy = storage.ExclusionViolationErrorOnMoreRecent
-		default:
-			e.Fatalf("unknown moreRecentPolicy: %s", moreRecentPolicyStr)
-		}
+		opts.FailOnMoreRecent = true
 	}
-
 	opts.Uncertainty = uncertainty.Interval{
 		GlobalLimit: e.getTsWithName("globalUncertaintyLimit"),
 		LocalLimit:  hlc.ClockTimestamp(e.getTsWithName("localUncertaintyLimit")),

pkg/storage/pebble_mvcc_scanner.go

@@ -444,7 +444,7 @@ type pebbleMVCCScanner struct {
 	// extended encoding of any returned value. This includes the
 	// MVCCValueHeader.
 	rawMVCCValues    bool
-	moreRecentPolicy MVCCGetMoreRecentPolicy
+	failOnMoreRecent bool
 	keyBuf           []byte
 	savedBuf         []byte
 	lazyFetcherBuf   pebble.LazyFetcher
@@ -571,12 +571,12 @@ func (p *pebbleMVCCScanner) init(
 	// future-time timestamps with earlier local timestamps. We are only able
 	// to skip uncertainty checks if p.ts >= global_uncertainty_limit.
 	//
-	// We disable checkUncertainty when the scanner is configured with a
-	// moreRecentPolicy other than IgnoreMoreRecent. This avoids cases in which a
-	// scan would have failed with a WriteTooOldError or ExclusionViolationError
-	// but instead gets an unexpected ReadWithinUncertaintyIntervalError See:
+	// We disable checkUncertainty when the scanner is configured with failOnMoreRecent.
+	// This avoids cases in which a scan would have failed with a WriteTooOldError
+	// but instead gets an unexpected ReadWithinUncertaintyIntervalError
+	// See:
 	// https://github.com/cockroachdb/cockroach/issues/119681
-	p.checkUncertainty = p.ts.Less(p.uncertainty.GlobalLimit) && p.moreRecentPolicy == IgnoreMoreRecent
+	p.checkUncertainty = p.ts.Less(p.uncertainty.GlobalLimit) && !p.failOnMoreRecent
 }
 
 // get seeks to the start key exactly once and adds one KV to the result set.
@@ -793,15 +793,9 @@ func (p *pebbleMVCCScanner) maybeFailOnMoreRecent() {
 	if p.err != nil || p.mostRecentTS.IsEmpty() {
 		return
 	}
-
-	switch p.moreRecentPolicy {
-	case WriteTooOldErrorOnMoreRecent:
-		// The txn can't write at the existing timestamp, so we provide the error
-		// with the timestamp immediately after it.
-		p.err = kvpb.NewWriteTooOldError(p.ts, p.mostRecentTS.Next(), p.mostRecentKey)
-	case ExclusionViolationErrorOnMoreRecent:
-		p.err = kvpb.NewExclusionViolationError(p.ts, p.mostRecentTS, p.mostRecentKey)
-	}
+	// The txn can't write at the existing timestamp, so we provide the error
+	// with the timestamp immediately after it.
+	p.err = kvpb.NewWriteTooOldError(p.ts, p.mostRecentTS.Next(), p.mostRecentKey)
 	p.results.clear()
 	p.intents.Reset()
 }
@@ -867,7 +861,7 @@ func (p *pebbleMVCCScanner) getOne(ctx context.Context) (ok, added bool) {
 
 		// ts == read_ts
 		if p.curUnsafeKey.Timestamp == p.ts {
-			if p.moreRecentPolicy != IgnoreMoreRecent {
+			if p.failOnMoreRecent {
 				// 2. Our txn's read timestamp is equal to the most recent
 				// version's timestamp and the scanner has been configured to
 				// throw a write too old error on equal or more recent versions.
@@ -899,7 +893,7 @@ func (p *pebbleMVCCScanner) getOne(ctx context.Context) (ok, added bool) {
 		}
 
 		// ts > read_ts
-		if p.moreRecentPolicy != IgnoreMoreRecent {
+		if p.failOnMoreRecent {
 			// 4. Our txn's read timestamp is less than the most recent
 			// version's timestamp and the scanner has been configured to
 			// throw a write too old error on equal or more recent versions.
@@ -982,7 +976,7 @@ func (p *pebbleMVCCScanner) getOne(ctx context.Context) (ok, added bool) {
 
 	ownIntent := p.txn != nil && p.meta.Txn.ID.Equal(p.txn.ID)
 	if !ownIntent {
-		conflictingIntent := metaTS.LessEq(p.ts) || (p.moreRecentPolicy != IgnoreMoreRecent)
+		conflictingIntent := metaTS.LessEq(p.ts) || p.failOnMoreRecent
 		if !conflictingIntent {
 			// 8. The key contains an intent, but we're reading below the intent.
 			// Seek to the desired version, checking for uncertainty if necessary.
@@ -1578,7 +1572,7 @@ func (p *pebbleMVCCScanner) processRangeKeys(seeked bool, reverse bool) bool {
 			// Check for conflicts with range keys at or above the read timestamp.
 			// We don't need to handle e.g. skipLocked, because range keys don't
 			// currently have intents.
-			if p.moreRecentPolicy != IgnoreMoreRecent {
+			if p.failOnMoreRecent {
 				if key := p.parent.UnsafeKey(); !hasPoint || !key.Timestamp.IsEmpty() {
 					if newest := p.curRangeKeys.Newest(); p.ts.LessEq(newest) {
 						if p.mostRecentTS.Forward(newest) {

pkg/storage/pebble_mvcc_scanner_test.go

@@ -95,7 +95,7 @@ func TestMVCCScanWithManyVersionsAndSeparatedIntents(t *testing.T) {
 		ts:               ts,
 		inconsistent:     false,
 		tombstones:       false,
-		moreRecentPolicy: IgnoreMoreRecent,
+		failOnMoreRecent: false,
 	}
 	var results pebbleResults
 	mvccScanner.init(nil /* txn */, uncertainty.Interval{}, &results)

pkg/storage/testdata/mvcc_histories/read_fail_on_more_recent

@@ -21,7 +21,7 @@ meta: "k2"/0,0 -> txn={id=00000001 key=/Min iso=Serializable pri=0.00000000 epo=
 data: "k2"/10.000000000,0 -> /BYTES/v
 
 # Test cases:
-#
+# 
 # for k in (k1, k2):
 #   for op in (get, scan):
 #     for ts in (9, 10, 11):
@@ -51,12 +51,6 @@ get k=k1 ts=10,0 failOnMoreRecent
 get: "k1" -> <no data>
 error: (*kvpb.WriteTooOldError:) WriteTooOldError: write for key "k1" at timestamp 10.000000000,0 too old; must write at or above 10.000000000,1
 
-run error
-get k=k1 ts=10,0 moreRecentPolicy=exclusionViolationErrorOnMoreRecent
-----
-get: "k1" -> <no data>
-error: (*kvpb.ExclusionViolationError:) write exclusion on key "k1" expected since 10.000000000,0 but found write at 10.000000000,0
-
 run ok
 get k=k1 ts=11,0
 ----
@@ -170,7 +164,7 @@ scan: "k2"-"k3" -> <no data>
 error: (*kvpb.LockConflictError:) conflicting locks on "k2"
 
 # More test cases:
-#
+# 
 # span = [k1, k3)
 # op   = scan
 # for ts in (9, 10, 11):
@@ -221,12 +215,6 @@ get k=k1 ts=9,0 inconsistent failOnMoreRecent
 get: "k1" -> <no data>
 error: (*withstack.withStack:) cannot allow inconsistent reads with fail on more recent option
 
-run error
-get k=k1 ts=9,0 inconsistent moreRecentPolicy=exclusionViolationErrorOnMoreRecent
-----
-get: "k1" -> <no data>
-error: (*withstack.withStack:) cannot allow inconsistent reads with fail on more recent option
-
 run error
 scan k=k1 end=k2 ts=9,0 inconsistent failOnMoreRecent
 ----