kvstorage: make replica destruction always sorted

Epic: none
Release note: none

Author: pav-kv

pkg/kv/kvserver/client_merge_test.go

@@ -3957,18 +3957,17 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 				sstFile := &storage.MemObject{}
 				sst := storage.MakeIngestionSSTWriter(ctx, st, sstFile)
 				defer sst.Close()
-				{
-					// The snapshot code uses ClearRangeWithHeuristic with a threshold of
-					// 1 to clear the range, but it will truncate the range tombstone to
-					// the first key. In this case, the first key is RangeGCThresholdKey,
-					// which doesn't yet exist in the engine, so we set it manually.
-					sl := rditer.Select(rangeID, rditer.SelectOpts{
-						ReplicatedByRangeID: true,
-					})
-					require.Len(t, sl, 1)
-					s := sl[0]
-					require.NoError(t, sst.ClearRawRange(keys.RangeGCThresholdKey(rangeID), s.EndKey, true, false))
-				}
+				// The snapshot code uses ClearRangeWithHeuristic with a threshold of 1
+				// to clear the range, but it will truncate the range tombstone to the
+				// first key. In this case, the first key is RangeGCThresholdKey, which
+				// doesn't yet exist in the engine, so we set it manually.
+				//
+				// The deletion also extends to the RangeTombstoneKey.
+				require.NoError(t, sst.ClearRawRange(
+					keys.RangeGCThresholdKey(rangeID),
+					keys.RangeTombstoneKey(rangeID),
+					true, false,
+				))
 				require.NoError(t, kvstorage.MakeStateLoader(rangeID).SetRangeTombstone(
 					context.Background(), &sst,
 					kvserverpb.RangeTombstone{NextReplicaID: math.MaxInt32},

pkg/kv/kvserver/kvstorage/destroy.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"math"
 
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/rditer"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -77,14 +78,17 @@ type DestroyReplicaInfo struct {
 // DestroyReplica destroys the entirety of the replica's state in storage, and
 // installs a RangeTombstone in its place. It handles both uninitialized and
 // initialized replicas uniformly.
+//
+// This call issues all writes ordered by key. This is to support a large
+// variety of uses, including SSTable generation for snapshot application.
 func DestroyReplica(
 	ctx context.Context,
 	reader storage.Reader,
 	writer storage.Writer,
 	info DestroyReplicaInfo,
 	next roachpb.ReplicaID,
 ) error {
-	return destroyReplicaImpl(ctx, reader, writer, info, next, false /* forceSortedKeys */)
+	return destroyReplicaImpl(ctx, reader, writer, info, next)
 }
 
 func destroyReplicaImpl(
@@ -93,7 +97,6 @@ func destroyReplicaImpl(
 	writer storage.Writer,
 	info DestroyReplicaInfo,
 	next roachpb.ReplicaID,
-	forceSortedKeys bool,
 ) error {
 	if next <= info.ReplicaID {
 		return errors.AssertionFailedf("%v must not survive its own tombstone", info.FullReplicaID)
@@ -114,62 +117,82 @@ func destroyReplicaImpl(
 			info.FullReplicaID, ts.NextReplicaID, next)
 	}
 
-	_ = DestroyReplicaTODO // 2.1 + 2.2 + 3.1
-	// NB: if forceSortedKeys is true, disable the heuristic and force deletion by
-	// range keys. This call can be used for generating SSTables when ingesting a
-	// snapshot, which requires Clears and Puts to be written in key order. Since
-	// we write RangeTombstoneKey further down, ensure that this is the only point
-	// key here.
-	pointKeyThreshold := ClearRangeThresholdPointKeys()
-	if forceSortedKeys {
-		pointKeyThreshold = 1
+	_ = DestroyReplicaTODO
+	// Clear all range data in sorted order of engine keys. This call can be used
+	// for generating SSTables when ingesting a snapshot, which requires Clears
+	// and Puts to be written in key order.
+	//
+	// All the code below is equivalent to clearing all the spans that the
+	// following SelectOpts represents, and leaving only the RangeTombstoneKey
+	// populated with the specified NextReplicaID.
+	if buildutil.CrdbTestBuild {
+		_ = rditer.SelectOpts{
+			ReplicatedByRangeID:   true,
+			UnreplicatedByRangeID: true,
+			Ranged:                rditer.SelectAllRanged(info.Keys),
+		}
+	}
+
+	// First, clear all RangeID-local replicated keys. Also, include all
+	// RangeID-local unreplicated keys < RangeTombstoneKey as a drive-by, since we
+	// decided that these (currently none) belong to the state machine engine.
+	tombstoneKey := keys.RangeTombstoneKey(info.RangeID)
+	if err := storage.ClearRangeWithHeuristic(
+		ctx, reader, writer,
+		keys.MakeRangeIDReplicatedPrefix(info.RangeID),
+		tombstoneKey,
+		ClearRangeThresholdPointKeys(),
+	); err != nil {
+		return err
+	}
+	// Save a tombstone to ensure that replica IDs never get reused.
+	if err := sl.SetRangeTombstone(ctx, writer, kvserverpb.RangeTombstone{
+		NextReplicaID: next, // NB: NextReplicaID > 0
+	}); err != nil {
+		return err
 	}
-	// TODO(pav-kv): decompose this loop to delete raft and state machine keys
-	// separately. The main challenge is the unreplicated span because it is
-	// scattered across 2 engines.
+	// Clear the rest of the RangeID-local unreplicated keys.
+	// TODO(pav-kv): decompose this further to delete raft and state machine keys
+	// separately. Currently, all the remaining keys in this span belong to the
+	// raft engine except the RaftReplicaID.
+	if err := storage.ClearRangeWithHeuristic(
+		ctx, reader, writer,
+		tombstoneKey.Next(),
+		keys.MakeRangeIDUnreplicatedPrefix(info.RangeID).PrefixEnd(),
+		ClearRangeThresholdPointKeys(),
+	); err != nil {
+		return err
+	}
+	// Finally, clear all the user keys (MVCC keyspace and the corresponding
+	// system and lock table keys), if info.Keys is not empty.
 	for _, span := range rditer.Select(info.RangeID, rditer.SelectOpts{
-		UnreplicatedByRangeID: true,
-		ReplicatedByRangeID:   true,
-		Ranged:                rditer.SelectAllRanged(info.Keys),
+		Ranged: rditer.SelectAllRanged(info.Keys),
 	}) {
 		if err := storage.ClearRangeWithHeuristic(
-			ctx, reader, writer, span.Key, span.EndKey, pointKeyThreshold,
+			ctx, reader, writer, span.Key, span.EndKey, ClearRangeThresholdPointKeys(),
 		); err != nil {
 			return err
 		}
 	}
-
-	// Save a tombstone to ensure that replica IDs never get reused.
-	_ = DestroyReplicaTODO // 2.3
-	return sl.SetRangeTombstone(ctx, writer, kvserverpb.RangeTombstone{
-		NextReplicaID: next, // NB: NextReplicaID > 0
-	})
+	return nil
 }
 
 // SubsumeReplica is like DestroyReplica, but it does not delete the user keys
 // (and the corresponding system and lock table keys). The latter are inherited
 // by the subsuming range.
 //
-// The forceSortedKeys flag, if true, forces the writes to be generated in the
-// sorted order of keys. This is to support feeding the writes from this
-// function to SSTables, in the snapshot ingestion path.
-//
 // Returns SelectOpts which can be used to reflect on the key spans that this
 // function clears.
 // TODO(pav-kv): get rid of SelectOpts.
 func SubsumeReplica(
-	ctx context.Context,
-	reader storage.Reader,
-	writer storage.Writer,
-	info DestroyReplicaInfo,
-	forceSortedKeys bool,
+	ctx context.Context, reader storage.Reader, writer storage.Writer, info DestroyReplicaInfo,
 ) (rditer.SelectOpts, error) {
 	// Forget about the user keys.
 	info.Keys = roachpb.RSpan{}
 	return rditer.SelectOpts{
 		ReplicatedByRangeID:   true,
 		UnreplicatedByRangeID: true,
-	}, destroyReplicaImpl(ctx, reader, writer, info, MergedTombstoneReplicaID, forceSortedKeys)
+	}, destroyReplicaImpl(ctx, reader, writer, info, MergedTombstoneReplicaID)
 }
 
 // RemoveStaleRHSFromSplit removes all data for the RHS replica of a split. This

pkg/kv/kvserver/kvstorage/testdata/TestDestroyReplica.txt

@@ -19,7 +19,7 @@ Put: "a"/Intent/00000000-0000-0000-0000-000000000000:
 Put: 0.000000001,0 "y\xff" (0x79ff00000000000000000109): ""
 Put: "y\xff"/Intent/00000000-0000-0000-0000-000000000000: 
 >> destroy:
-Delete (Sized at 31): 0,0 /Local/RangeID/123/u/RangeTombstone (0x0169f67b757266746200): 
+Put: 0,0 /Local/RangeID/123/u/RangeTombstone (0x0169f67b757266746200): next_replica_id:4 
 Delete (Sized at 39): 0,0 /Local/RangeID/123/u/RaftHardState (0x0169f67b757266746800): 
 Delete (Sized at 43): 0,0 /Local/RangeID/123/u/RaftLog/logIndex:11 (0x0169f67b757266746c000000000000000b00): 
 Delete (Sized at 43): 0,0 /Local/RangeID/123/u/RaftLog/logIndex:12 (0x0169f67b757266746c000000000000000c00): 
@@ -35,4 +35,3 @@ Delete (Sized at 26): /Local/Lock"a" 0300000000000000000000000000000000 (0x017a6
 Delete (Sized at 27): /Local/Lock"y\xff" 0300000000000000000000000000000000 (0x017a6b1279ff000100030000000000000000000000000000000012): 
 Delete (Sized at 11): 0.000000001,0 "a" (0x6100000000000000000109): 
 Delete (Sized at 12): 0.000000001,0 "y\xff" (0x79ff00000000000000000109): 
-Put: 0,0 /Local/RangeID/123/u/RangeTombstone (0x0169f67b757266746200): next_replica_id:4 

pkg/kv/kvserver/replica_app_batch.go

@@ -360,7 +360,7 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		rhsRepl.readOnlyCmdMu.Unlock()
 
 		if _, err := kvstorage.SubsumeReplica(
-			ctx, b.batch, b.batch, rhsRepl.destroyInfoRaftMuLocked(), false, /* forceSortedKeys */
+			ctx, b.batch, b.batch, rhsRepl.destroyInfoRaftMuLocked(),
 		); err != nil {
 			return errors.Wrapf(err, "unable to subsume replica before merge")
 		}

pkg/kv/kvserver/snapshot_apply_prepare.go

@@ -149,7 +149,7 @@ func (s *snapWriteBuilder) clearSubsumedReplicaDiskData(ctx context.Context) err
 	for _, sub := range s.subsume {
 		// We have to create an SST for the subsumed replica's range-id local keys.
 		if err := s.writeSST(ctx, func(ctx context.Context, w storage.Writer) error {
-			opts, err := kvstorage.SubsumeReplica(ctx, reader, w, sub, true /* forceSortedKeys */)
+			opts, err := kvstorage.SubsumeReplica(ctx, reader, w, sub)
 			s.cleared = append(s.cleared, rditer.Select(sub.RangeID, opts)...)
 			return err
 		}); err != nil {

pkg/kv/kvserver/testdata/TestPrepareSnapApply.txt

@@ -6,11 +6,11 @@ Put: 0,0 /Local/RangeID/123/u/RaftHardState (0x0169f67b757266746800): term:20 vo
 Put: 0,0 /Local/RangeID/123/u/RaftReplicaID (0x0169f67b757266747200): replica_id:4 
 Put: 0,0 /Local/RangeID/123/u/RaftTruncatedState (0x0169f67b757266747400): index:100 term:20 
 >> sst:
-Delete Range: [0,0 /Local/RangeID/101/u/RaftReplicaID (0x0169ed757266747200): , 0,0 /Local/RangeID/101/v"" (0x0169ed7600): )
 Put: 0,0 /Local/RangeID/101/u/RangeTombstone (0x0169ed757266746200): next_replica_id:2147483647 
+Delete (Sized at 30): 0,0 /Local/RangeID/101/u/RaftReplicaID (0x0169ed757266747200): 
 >> sst:
-Delete Range: [0,0 /Local/RangeID/102/u/RaftReplicaID (0x0169ee757266747200): , 0,0 /Local/RangeID/102/v"" (0x0169ee7600): )
 Put: 0,0 /Local/RangeID/102/u/RangeTombstone (0x0169ee757266746200): next_replica_id:2147483647 
+Delete (Sized at 30): 0,0 /Local/RangeID/102/u/RaftReplicaID (0x0169ee757266747200): 
 >> sst:
 Delete (Sized at 27): /Local/Lock"y\xff" 0300000000000000000000000000000000 (0x017a6b1279ff000100030000000000000000000000000000000012): 
 >> sst: