Merge cockroachdb#44379 cockroachdb#44554

44379: kvnemesis: enable AdminMerge testing r=nvanbenschoten a=danhhz

It was not enabled originally because of a bug in RangeFeed (cockroachdb#43967) but
this got fixed in cockroachdb#44035.

Release note: None

44554: keys,storage: delete all references to RaftLastIndexKey r=irfansharif a=irfansharif

Our last usage of RaftLastIndexKey was way back in cockroachdb#18717. This was
deprecated then, though never marked as such. It's safe for removal
seeing as how it was an unreplicated key to begin with. We also no
longer have code looking through RaftLastIndexKey (instead we reverse
scan raft log entries to determine the last index).

Release note: None

Co-authored-by: Daniel Harrison <[email protected]>
Co-authored-by: irfan sharif <[email protected]>

pkg/keys/constants.go

@@ -76,9 +76,8 @@ var (
 	// AbortSpan protects a transaction from re-reading its own intents
 	// after it's been aborted.
 	LocalAbortSpanSuffix = []byte("abc-")
-	// LocalRangeFrozenStatusSuffix is the suffix for a frozen status.
-	// No longer used; exists only to reserve the key so we don't use it.
-	LocalRangeFrozenStatusSuffix = []byte("fzn-")
+	// localRangeFrozenStatusSuffix is DEPRECATED and remains to prevent reuse.
+	localRangeFrozenStatusSuffix = []byte("fzn-")
 	// LocalRangeLastGCSuffix is the suffix for the last GC.
 	LocalRangeLastGCSuffix = []byte("lgc-")
 	// LocalRangeAppliedStateSuffix is the suffix for the range applied state
@@ -100,10 +99,8 @@ var (
 	LocalLeaseAppliedIndexLegacySuffix = []byte("rlla")
 	// LocalRangeStatsLegacySuffix is the suffix for range statistics.
 	LocalRangeStatsLegacySuffix = []byte("stat")
-	// LocalTxnSpanGCThresholdSuffix is the suffix for the last txn span GC's
-	// threshold. No longer used; exists only to reserve the key so we don't use
-	// it.
-	LocalTxnSpanGCThresholdSuffix = []byte("tst-")
+	// localTxnSpanGCThresholdSuffix is DEPRECATED and remains to prevent reuse.
+	localTxnSpanGCThresholdSuffix = []byte("tst-")
 
 	// 2. Unreplicated Range-ID keys
 	//
@@ -115,17 +112,16 @@ var (
 	localRangeIDUnreplicatedInfix = []byte("u")
 	// LocalRaftHardStateSuffix is the Suffix for the raft HardState.
 	LocalRaftHardStateSuffix = []byte("rfth")
-	// LocalRaftLastIndexSuffix is the suffix for raft's last index.
-	LocalRaftLastIndexSuffix = []byte("rfti")
+	// localRaftLastIndexSuffix is DEPRECATED and remains to prevent reuse.
+	localRaftLastIndexSuffix = []byte("rfti")
 	// LocalRaftLogSuffix is the suffix for the raft log.
 	LocalRaftLogSuffix = []byte("rftl")
 	// LocalRangeLastReplicaGCTimestampSuffix is the suffix for a range's last
 	// replica GC timestamp (for GC of old replicas).
 	LocalRangeLastReplicaGCTimestampSuffix = []byte("rlrt")
-	// LocalRangeLastVerificationTimestampSuffixDeprecated is the suffix for a
-	// range's last verification timestamp (for checking integrity of on-disk
-	// data). Note: DEPRECATED.
-	LocalRangeLastVerificationTimestampSuffixDeprecated = []byte("rlvt")
+	// localRangeLastVerificationTimestampSuffix is DEPRECATED and remains to
+	// prevent reuse.
+	localRangeLastVerificationTimestampSuffix = []byte("rlvt")
 
 	// 3. Range local keys
 	//
@@ -182,10 +178,9 @@ var (
 	// is to allow a restarting node to discover approximately how long it has
 	// been down without needing to retrieve liveness records from the cluster.
 	localStoreLastUpSuffix = []byte("uptm")
-	//
-	// localRemovedLeakedRaftEntriesSuffix is DEPRECATED and remains to prevent reuse.
+	// localRemovedLeakedRaftEntriesSuffix is DEPRECATED and remains to prevent
+	// reuse.
 	localRemovedLeakedRaftEntriesSuffix = []byte("dlre")
-	_                                   = localRemovedLeakedRaftEntriesSuffix
 	// LocalStoreSuggestedCompactionsMin is the start of the span of
 	// possible suggested compaction keys for a store.
 	LocalStoreSuggestedCompactionsMin = MakeStoreKey(localStoreSuggestedCompactionSuffix, nil)

pkg/keys/doc.go

@@ -141,7 +141,7 @@ package keys
 
 // NB: The sorting order of the symbols below map to the physical layout.
 // Preserve group-wise ordering when adding new constants.
-var keymap = [...]interface{}{ // lint:ignore U1001
+var _ = [...]interface{}{
 	MinKey,
 
 	// There are four types of local key data enumerated below: replicated
@@ -168,7 +168,6 @@ var keymap = [...]interface{}{ // lint:ignore U1001
 	//   Typical examples are MVCC stats and the abort span. They all share
 	//   `LocalRangeIDPrefix` and `LocalRangeIDReplicatedInfix`.
 	AbortSpanKey,                    // "abc-"
-	RangeFrozenStatusKey,            // "fzn-"
 	RangeLastGCKey,                  // "lgc-"
 	RangeAppliedStateKey,            // "rask"
 	RaftAppliedIndexLegacyKey,       // "rfta"
@@ -177,19 +176,16 @@ var keymap = [...]interface{}{ // lint:ignore U1001
 	RangeLeaseKey,                   // "rll-"
 	LeaseAppliedIndexLegacyKey,      // "rlla"
 	RangeStatsLegacyKey,             // "stat"
-	RangeTxnSpanGCThresholdKey,      // "tst-"
 
 	//   2. Unreplicated range-ID local keys: These contain metadata that
 	//   pertain to just one replica of a range. They are unreplicated and
 	//   unaddressable. The typical example is the Raft log. They all share
 	//   `LocalRangeIDPrefix` and `localRangeIDUnreplicatedInfix`.
 	RaftTombstoneKey,               // "rftb"
 	RaftHardStateKey,               // "rfth"
-	RaftLastIndexKey,               // "rfti"
 	RaftLogKey,                     // "rftl"
 	RaftTruncatedStateKey,          // "rftt"
 	RangeLastReplicaGCTimestampKey, // "rlrt"
-	RangeLastVerificationTimestampKeyDeprecated, // "rlvt"
 
 	//   3. Range local keys: These also store metadata that pertains to a range
 	//   as a whole. They are replicated and addressable. Typical examples are
@@ -242,3 +238,12 @@ var keymap = [...]interface{}{ // lint:ignore U1001
 
 	MaxKey,
 }
+
+// Unused, deprecated keys.
+var _ = [...]interface{}{
+	localRaftLastIndexSuffix,
+	localRangeFrozenStatusSuffix,
+	localRangeLastVerificationTimestampSuffix,
+	localRemovedLeakedRaftEntriesSuffix,
+	localTxnSpanGCThresholdSuffix,
+}

pkg/keys/keys.go

@@ -254,11 +254,6 @@ func RaftTruncatedStateLegacyKey(rangeID roachpb.RangeID) roachpb.Key {
 	return MakeRangeIDPrefixBuf(rangeID).RaftTruncatedStateLegacyKey()
 }
 
-// RangeFrozenStatusKey returns a system-local key for the frozen status.
-func RangeFrozenStatusKey(rangeID roachpb.RangeID) roachpb.Key {
-	return MakeRangeIDPrefixBuf(rangeID).RangeFrozenStatusKey()
-}
-
 // RangeLeaseKey returns a system-local key for a range lease.
 func RangeLeaseKey(rangeID roachpb.RangeID) roachpb.Key {
 	return MakeRangeIDPrefixBuf(rangeID).RangeLeaseKey()
@@ -279,12 +274,6 @@ func RangeLastGCKey(rangeID roachpb.RangeID) roachpb.Key {
 	return MakeRangeIDPrefixBuf(rangeID).RangeLastGCKey()
 }
 
-// RangeTxnSpanGCThresholdKey returns a system-local key for last used GC
-// threshold on the txn span.
-func RangeTxnSpanGCThresholdKey(rangeID roachpb.RangeID) roachpb.Key {
-	return MakeRangeIDPrefixBuf(rangeID).RangeTxnSpanGCThresholdKey()
-}
-
 // MakeRangeIDUnreplicatedPrefix creates a range-local key prefix from
 // rangeID for all unreplicated data.
 func MakeRangeIDUnreplicatedPrefix(rangeID roachpb.RangeID) roachpb.Key {
@@ -321,12 +310,6 @@ func RaftHardStateKey(rangeID roachpb.RangeID) roachpb.Key {
 	return MakeRangeIDPrefixBuf(rangeID).RaftHardStateKey()
 }
 
-// RaftLastIndexKey returns a system-local key for the last index of the
-// Raft log.
-func RaftLastIndexKey(rangeID roachpb.RangeID) roachpb.Key {
-	return MakeRangeIDPrefixBuf(rangeID).RaftLastIndexKey()
-}
-
 // RaftLogPrefix returns the system-local prefix shared by all Entries
 // in a Raft log.
 func RaftLogPrefix(rangeID roachpb.RangeID) roachpb.Key {
@@ -344,12 +327,6 @@ func RangeLastReplicaGCTimestampKey(rangeID roachpb.RangeID) roachpb.Key {
 	return MakeRangeIDPrefixBuf(rangeID).RangeLastReplicaGCTimestampKey()
 }
 
-// RangeLastVerificationTimestampKeyDeprecated returns a range-local
-// key for the range's last verification timestamp.
-func RangeLastVerificationTimestampKeyDeprecated(rangeID roachpb.RangeID) roachpb.Key {
-	return MakeRangeIDPrefixBuf(rangeID).RangeLastVerificationTimestampKeyDeprecated()
-}
-
 // MakeRangeKey creates a range-local key based on the range
 // start key, metadata key suffix, and optional detail (e.g. the
 // transaction ID for a txn record, etc.).
@@ -917,11 +894,6 @@ func (b RangeIDPrefixBuf) RaftTruncatedStateLegacyKey() roachpb.Key {
 	return append(b.replicatedPrefix(), LocalRaftTruncatedStateLegacySuffix...)
 }
 
-// RangeFrozenStatusKey returns a system-local key for the frozen status.
-func (b RangeIDPrefixBuf) RangeFrozenStatusKey() roachpb.Key {
-	return append(b.replicatedPrefix(), LocalRangeFrozenStatusSuffix...)
-}
-
 // RangeLeaseKey returns a system-local key for a range lease.
 func (b RangeIDPrefixBuf) RangeLeaseKey() roachpb.Key {
 	return append(b.replicatedPrefix(), LocalRangeLeaseSuffix...)
@@ -939,12 +911,6 @@ func (b RangeIDPrefixBuf) RangeLastGCKey() roachpb.Key {
 	return append(b.replicatedPrefix(), LocalRangeLastGCSuffix...)
 }
 
-// RangeTxnSpanGCThresholdKey returns a system-local key for last used GC
-// threshold on the txn span.
-func (b RangeIDPrefixBuf) RangeTxnSpanGCThresholdKey() roachpb.Key {
-	return append(b.replicatedPrefix(), LocalTxnSpanGCThresholdSuffix...)
-}
-
 // RaftTombstoneKey returns a system-local key for a raft tombstone.
 func (b RangeIDPrefixBuf) RaftTombstoneKey() roachpb.Key {
 	return append(b.unreplicatedPrefix(), LocalRaftTombstoneSuffix...)
@@ -960,12 +926,6 @@ func (b RangeIDPrefixBuf) RaftHardStateKey() roachpb.Key {
 	return append(b.unreplicatedPrefix(), LocalRaftHardStateSuffix...)
 }
 
-// RaftLastIndexKey returns a system-local key for the last index of the
-// Raft log.
-func (b RangeIDPrefixBuf) RaftLastIndexKey() roachpb.Key {
-	return append(b.unreplicatedPrefix(), LocalRaftLastIndexSuffix...)
-}
-
 // RaftLogPrefix returns the system-local prefix shared by all Entries
 // in a Raft log.
 func (b RangeIDPrefixBuf) RaftLogPrefix() roachpb.Key {
@@ -983,12 +943,6 @@ func (b RangeIDPrefixBuf) RangeLastReplicaGCTimestampKey() roachpb.Key {
 	return append(b.unreplicatedPrefix(), LocalRangeLastReplicaGCTimestampSuffix...)
 }
 
-// RangeLastVerificationTimestampKeyDeprecated returns a range-local
-// key for the range's last verification timestamp.
-func (b RangeIDPrefixBuf) RangeLastVerificationTimestampKeyDeprecated() roachpb.Key {
-	return append(b.unreplicatedPrefix(), LocalRangeLastVerificationTimestampSuffixDeprecated...)
-}
-
 // ZoneKeyPrefix returns the key prefix for id's row in the system.zones table.
 func ZoneKeyPrefix(id uint32) roachpb.Key {
 	k := MakeTablePrefix(uint32(ZonesTableID))

pkg/keys/keys_test.go

@@ -146,12 +146,9 @@ func TestKeyAddressError(t *testing.T) {
 			RangeLeaseKey(0),
 			RangeStatsLegacyKey(0),
 			RaftHardStateKey(0),
-			RaftLastIndexKey(0),
 			RaftLogPrefix(0),
 			RaftLogKey(0, 0),
 			RangeLastReplicaGCTimestampKey(0),
-			RangeLastVerificationTimestampKeyDeprecated(0),
-			RangeDescriptorKey(roachpb.RKey(RangeLastVerificationTimestampKeyDeprecated(0))),
 		},
 		"local key .* malformed": {
 			makeKey(localPrefix, roachpb.Key("z")),

pkg/keys/printer.go

@@ -161,13 +161,9 @@ var (
 			psFunc: raftLogKeyParse,
 		},
 		{name: "RaftTruncatedState", suffix: LocalRaftTruncatedStateLegacySuffix},
-		{name: "RaftLastIndex", suffix: LocalRaftLastIndexSuffix},
 		{name: "RangeLastReplicaGCTimestamp", suffix: LocalRangeLastReplicaGCTimestampSuffix},
-		{name: "RangeLastVerificationTimestamp", suffix: LocalRangeLastVerificationTimestampSuffixDeprecated},
 		{name: "RangeLease", suffix: LocalRangeLeaseSuffix},
 		{name: "RangeStats", suffix: LocalRangeStatsLegacySuffix},
-		{name: "RangeTxnSpanGCThreshold", suffix: LocalTxnSpanGCThresholdSuffix},
-		{name: "RangeFrozenStatus", suffix: LocalRangeFrozenStatusSuffix},
 		{name: "RangeLastGC", suffix: LocalRangeLastGCSuffix},
 	}
 

pkg/keys/printer_test.go

@@ -66,16 +66,12 @@ func TestPrettyPrint(t *testing.T) {
 		{keys.RaftTruncatedStateKey(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/u/RaftTruncatedState", revertSupportUnknown},
 		{keys.RangeLeaseKey(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/r/RangeLease", revertSupportUnknown},
 		{keys.RangeStatsLegacyKey(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/r/RangeStats", revertSupportUnknown},
-		{keys.RangeTxnSpanGCThresholdKey(roachpb.RangeID(1000001)), `/Local/RangeID/1000001/r/RangeTxnSpanGCThreshold`, revertSupportUnknown},
-		{keys.RangeFrozenStatusKey(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/r/RangeFrozenStatus", revertSupportUnknown},
 		{keys.RangeLastGCKey(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/r/RangeLastGC", revertSupportUnknown},
 
 		{keys.RaftHardStateKey(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/u/RaftHardState", revertSupportUnknown},
-		{keys.RaftLastIndexKey(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/u/RaftLastIndex", revertSupportUnknown},
 		{keys.RaftTombstoneKey(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/u/RaftTombstone", revertSupportUnknown},
 		{keys.RaftLogKey(roachpb.RangeID(1000001), uint64(200001)), "/Local/RangeID/1000001/u/RaftLog/logIndex:200001", revertSupportUnknown},
 		{keys.RangeLastReplicaGCTimestampKey(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/u/RangeLastReplicaGCTimestamp", revertSupportUnknown},
-		{keys.RangeLastVerificationTimestampKeyDeprecated(roachpb.RangeID(1000001)), "/Local/RangeID/1000001/u/RangeLastVerificationTimestamp", revertSupportUnknown},
 
 		{keys.MakeRangeKeyPrefix(roachpb.RKey(keys.MakeTablePrefix(42))), `/Local/Range/Table/42`, revertSupportUnknown},
 		{keys.RangeDescriptorKey(roachpb.RKey(keys.MakeTablePrefix(42))), `/Local/Range/Table/42/RangeDescriptor`, revertSupportUnknown},

pkg/kv/kvnemesis/applier_test.go

@@ -96,8 +96,10 @@ db.Txn(ctx, func(ctx context.Context, txn *client.Txn) error {
 		`)
 
 	// Splits and merges
-	check(t, step(split(`foo`)), `db.Split(ctx, "foo") // nil`)
-	check(t, step(merge(`foo`)), `db.Merge(ctx, "foo") // nil`)
-	checkErr(t, step(split(`foo`)), `db.Split(ctx, "foo") // aborted in distSender: context canceled`)
-	checkErr(t, step(merge(`foo`)), `db.Merge(ctx, "foo") // aborted in distSender: context canceled`)
+	check(t, step(split(`foo`)), `db.AdminSplit(ctx, "foo") // nil`)
+	check(t, step(merge(`foo`)), `db.AdminMerge(ctx, "foo") // nil`)
+	checkErr(t, step(split(`foo`)),
+		`db.AdminSplit(ctx, "foo") // aborted in distSender: context canceled`)
+	checkErr(t, step(merge(`foo`)),
+		`db.AdminMerge(ctx, "foo") // aborted in distSender: context canceled`)
 }

pkg/kv/kvnemesis/kvnemesis_test.go

@@ -53,17 +53,12 @@ func TestKVNemesisSingleNode(t *testing.T) {
 		// nontransactional batch are disjoint and upgrading to a transactional
 		// batch (see CrossRangeTxnWrapperSender) if they are. roachpb.SpanGroup can
 		// be used to efficiently check this.
-		OpPBatch:      0,
-		OpPClosureTxn: 10,
-		OpPSplitNew:   1,
-		OpPSplitAgain: 1,
-		// TODO(dan): Merge seems to occasionally be hanging, presumably because the
-		// merge txn is restarting. Investigate.
-		//
-		// TODO(dan): "merge failed: unexpected value". Nemeses's first bug find?
-		//
-		// OpPMergeNotSplit: 1,
-		// OpPMergeIsSplit:  1,
+		OpPBatch:         0,
+		OpPClosureTxn:    10,
+		OpPSplitNew:      1,
+		OpPSplitAgain:    1,
+		OpPMergeNotSplit: 1,
+		OpPMergeIsSplit:  1,
 	}
 
 	rng, _ := randutil.NewPseudoRand()

pkg/kv/kvnemesis/operations.go

@@ -159,12 +159,12 @@ func (op PutOperation) format(w *strings.Builder, fctx formatCtx) {
 }
 
 func (op SplitOperation) format(w *strings.Builder) {
-	fmt.Fprintf(w, `db.Split(ctx, %s)`, roachpb.Key(op.Key))
+	fmt.Fprintf(w, `db.AdminSplit(ctx, %s)`, roachpb.Key(op.Key))
 	op.Result.format(w)
 }
 
 func (op MergeOperation) format(w *strings.Builder) {
-	fmt.Fprintf(w, `db.Merge(ctx, %s)`, roachpb.Key(op.Key))
+	fmt.Fprintf(w, `db.AdminMerge(ctx, %s)`, roachpb.Key(op.Key))
 	op.Result.format(w)
 }
 

pkg/kv/kvnemesis/validator.go

@@ -129,7 +129,29 @@ func (v *validator) processOp(txnID *string, op Operation) {
 	case *SplitOperation:
 		v.failIfError(op, t.Result)
 	case *MergeOperation:
-		v.failIfError(op, t.Result)
+		if resultIsError(t.Result, `cannot merge final range`) {
+			// Because of some non-determinism, it is not worth it (or maybe not
+			// possible) to prevent these usage errors. Additionally, I (dan) think
+			// this hints at some unnecessary friction in the AdminMerge api. There is
+			// a similar inconsistency when a race condition means that AdminMerge is
+			// called on something that is not a split point. I propose that the
+			// AdminMerge contract should be that it can be called on any key, split
+			// point or not, and after a successful operation, the guarantee is that
+			// there is no split at that key. #44378
+			//
+			// In the meantime, no-op.
+		} else if resultIsError(t.Result, `merge failed: unexpected value`) {
+			// TODO(dan): If this error is going to remain a part of the kv API, we
+			// should make it sniffable with errors.As. Currently this seems to be
+			// broken by wrapping it with `roachpb.NewErrorf("merge failed: %s",
+			// err)`.
+			//
+			// However, I think the right thing to do is sniff this inside the
+			// AdminMerge code and retry so the client never sees it. In the meantime,
+			// no-op. #44377
+		} else {
+			v.failIfError(op, t.Result)
+		}
 	case *BatchOperation:
 		if !resultIsRetryable(t.Result) {
 			v.failIfError(op, t.Result)
@@ -245,6 +267,21 @@ func (v *validator) failIfError(op Operation, r Result) {
 	}
 }
 
+// TODO(dan): Checking errors using string containment is fragile at best and a
+// security issue at worst. Unfortunately, some errors that currently make it
+// out of our kv apis are created with `errors.New` and so do not have types
+// that can be sniffed. Some of these may be removed or handled differently but
+// the rest should graduate to documented parts of the public api. Remove this
+// once it happens.
+func resultIsError(r Result, msg string) bool {
+	if r.Type != ResultType_Error {
+		return false
+	}
+	ctx := context.Background()
+	err := errors.DecodeError(ctx, *r.Err)
+	return strings.Contains(err.Error(), msg)
+}
+
 func resultIsRetryable(r Result) bool {
 	if r.Type != ResultType_Error {
 		return false

pkg/storage/debug_print.go

@@ -257,13 +257,6 @@ func tryRangeIDKey(kv engine.MVCCKeyValue) (string, error) {
 		}
 		return strconv.FormatInt(i, 10), nil
 
-	case bytes.Equal(suffix, keys.LocalRangeFrozenStatusSuffix):
-		b, err := value.GetBool()
-		if err != nil {
-			return "", err
-		}
-		return strconv.FormatBool(b), nil
-
 	case bytes.Equal(suffix, keys.LocalAbortSpanSuffix):
 		msg = &roachpb.AbortSpanEntry{}
 
@@ -288,16 +281,6 @@ func tryRangeIDKey(kv engine.MVCCKeyValue) (string, error) {
 	case bytes.Equal(suffix, keys.LocalRaftHardStateSuffix):
 		msg = &raftpb.HardState{}
 
-	case bytes.Equal(suffix, keys.LocalRaftLastIndexSuffix):
-		i, err := value.GetInt()
-		if err != nil {
-			return "", err
-		}
-		return strconv.FormatInt(i, 10), nil
-
-	case bytes.Equal(suffix, keys.LocalRangeLastVerificationTimestampSuffixDeprecated):
-		msg = &hlc.Timestamp{}
-
 	case bytes.Equal(suffix, keys.LocalRangeLastReplicaGCTimestampSuffix):
 		msg = &hlc.Timestamp{}