Merge #155539

155539: kvstorage: encapsulate variants of replica destruction r=arulajmani a=pav-kv

This PR moves all variants of replica destruction writes to `kvstorage` package. It also decouples the `DestroyReplica` helper into two: one that fully removes the replica, and one that doesn't touch the user keys (`SubsumeReplica`).

Finally, `ClearRangeData` and its various options are no longer exposed outside the package, and become its implementation detail. `ClearRangeData` will likely be further broken down in a follow-up since it is not flexible enough for supporting separated raft storage.

Related to #152845

Related to #152845

Co-authored-by: Pavel Kalinnikov <pavel@cockroachlabs.com>

Author: craig[bot]

pkg/kv/kvserver/kvstorage/destroy.go

@@ -7,6 +7,7 @@ package kvstorage
 
 import (
 	"context"
+	"math"
 
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/rditer"
@@ -29,55 +30,63 @@ const (
 	// perhaps we should fix Pebble to handle large numbers of range tombstones in
 	// an sstable better.
 	ClearRangeThresholdPointKeys = 64
+
+	// MergedTombstoneReplicaID is the replica ID written into the RangeTombstone
+	// for replicas of a range which is known to have been merged. This value
+	// should prevent any messages from stale replicas of that range from ever
+	// resurrecting merged replicas. Whenever merging or subsuming a replica we
+	// know new replicas can never be created so this value is used even if we
+	// don't know the current replica ID.
+	MergedTombstoneReplicaID roachpb.ReplicaID = math.MaxInt32
 )
 
-// ClearRangeDataOptions specify which parts of a Replica are to be destroyed.
-type ClearRangeDataOptions struct {
-	// ClearReplicatedByRangeID indicates that replicated RangeID-based keys
+// clearRangeDataOptions specify which parts of a Replica are to be destroyed.
+type clearRangeDataOptions struct {
+	// clearReplicatedByRangeID indicates that replicated RangeID-based keys
 	// (abort span, etc) should be removed.
-	ClearReplicatedByRangeID bool
-	// ClearUnreplicatedByRangeID indicates that unreplicated RangeID-based keys
+	clearReplicatedByRangeID bool
+	// clearUnreplicatedByRangeID indicates that unreplicated RangeID-based keys
 	// (logstore state incl. HardState, etc) should be removed.
-	ClearUnreplicatedByRangeID bool
-	// ClearReplicatedBySpan causes the state machine data (i.e. the replicated state
+	clearUnreplicatedByRangeID bool
+	// clearReplicatedBySpan causes the state machine data (i.e. the replicated state
 	// for the given RSpan) that is key-addressable (i.e. range descriptor, user keys,
 	// locks) to be removed. No data is removed if this is the zero span.
-	ClearReplicatedBySpan roachpb.RSpan
+	clearReplicatedBySpan roachpb.RSpan
 
-	// If MustUseClearRange is true, a Pebble range tombstone will always be used
+	// If mustUseClearRange is true, a Pebble range tombstone will always be used
 	// to clear the key spans (unless empty). This is typically used when we need
 	// to write additional keys to an SST after this clear, e.g. a replica
 	// tombstone, since keys must be written in order. When this is false, a
 	// heuristic will be used instead.
-	MustUseClearRange bool
+	mustUseClearRange bool
 }
 
-// ClearRangeData clears the data associated with a range descriptor selected
+// clearRangeData clears the data associated with a range descriptor selected
 // by the provided options.
 //
 // TODO(tbg): could rename this to XReplica. The use of "Range" in both the
 // "CRDB Range" and "storage.ClearRange" context in the setting of this method could
 // be confusing.
-func ClearRangeData(
+func clearRangeData(
 	ctx context.Context,
 	rangeID roachpb.RangeID,
 	reader storage.Reader,
 	writer storage.Writer,
-	opts ClearRangeDataOptions,
+	opts clearRangeDataOptions,
 ) error {
 	keySpans := rditer.Select(rangeID, rditer.SelectOpts{
 		Ranged: rditer.SelectRangedOptions{
-			RSpan:      opts.ClearReplicatedBySpan,
+			RSpan:      opts.clearReplicatedBySpan,
 			SystemKeys: true,
 			LockTable:  true,
 			UserKeys:   true,
 		},
-		ReplicatedByRangeID:   opts.ClearReplicatedByRangeID,
-		UnreplicatedByRangeID: opts.ClearUnreplicatedByRangeID,
+		ReplicatedByRangeID:   opts.clearReplicatedByRangeID,
+		UnreplicatedByRangeID: opts.clearUnreplicatedByRangeID,
 	})
 
 	pointKeyThreshold := ClearRangeThresholdPointKeys
-	if opts.MustUseClearRange {
+	if opts.mustUseClearRange {
 		pointKeyThreshold = 1
 	}
 
@@ -120,21 +129,30 @@ type DestroyReplicaInfo struct {
 	Keys roachpb.RSpan
 }
 
-// DestroyReplica destroys all or a part of the Replica's state, installing a
-// RangeTombstone in its place. Due to merges, splits, etc, there is a need to
-// control which part of the state this method actually gets to remove, which is
-// done via the provided options[^1]; the caller is always responsible for
-// managing the remaining disk state accordingly.
-//
-// [^1] e.g., on a merge, the user data moves to the subsuming replica and must
-// not be cleared.
+// 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.
 func DestroyReplica(
 	ctx context.Context,
 	reader storage.Reader,
 	writer storage.Writer,
 	info DestroyReplicaInfo,
 	next roachpb.ReplicaID,
-	opts ClearRangeDataOptions,
+) error {
+	return destroyReplicaImpl(ctx, reader, writer, info, next, clearRangeDataOptions{
+		clearReplicatedByRangeID:   true,
+		clearUnreplicatedByRangeID: true,
+		clearReplicatedBySpan:      info.Keys,
+	})
+}
+
+func destroyReplicaImpl(
+	ctx context.Context,
+	reader storage.Reader,
+	writer storage.Writer,
+	info DestroyReplicaInfo,
+	next roachpb.ReplicaID,
+	opts clearRangeDataOptions,
 ) error {
 	if next <= info.ReplicaID {
 		return errors.AssertionFailedf("%v must not survive its own tombstone", info.FullReplicaID)
@@ -156,7 +174,7 @@ func DestroyReplica(
 	}
 
 	_ = DestroyReplicaTODO // 2.1 + 2.2 + 3.1
-	if err := ClearRangeData(ctx, info.RangeID, reader, writer, opts); err != nil {
+	if err := clearRangeData(ctx, info.RangeID, reader, writer, opts); err != nil {
 		return err
 	}
 	// Save a tombstone to ensure that replica IDs never get reused.
@@ -165,3 +183,73 @@ func DestroyReplica(
 		NextReplicaID: next, // NB: NextReplicaID > 0
 	})
 }
+
+// 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,
+) (rditer.SelectOpts, error) {
+	// NB: if required, set MustUseClearRange to true. This call can be used for
+	// generating SSTables when ingesting a snapshot, which requires Clears and
+	// Puts to be written in key order. DestroyReplica sets RangeTombstoneKey
+	// after clearing the unreplicated span which may contain higher keys.
+	opts := clearRangeDataOptions{
+		clearReplicatedByRangeID:   true,
+		clearUnreplicatedByRangeID: true,
+		mustUseClearRange:          forceSortedKeys,
+	}
+	return rditer.SelectOpts{
+		ReplicatedByRangeID:   opts.clearReplicatedByRangeID,
+		UnreplicatedByRangeID: opts.clearUnreplicatedByRangeID,
+	}, destroyReplicaImpl(ctx, reader, writer, info, MergedTombstoneReplicaID, opts)
+}
+
+// RemoveStaleRHSFromSplit removes all data for the RHS replica of a split. This
+// is used in a situation when the RHS replica is already known to have been
+// removed from our store, so any pending writes that were supposed to
+// initialize the RHS replica should be dropped from the write batch.
+//
+// TODO(#152199): do not remove the unreplicated state which can belong to a
+// newer (uninitialized) replica.
+func RemoveStaleRHSFromSplit(
+	ctx context.Context,
+	reader storage.Reader,
+	writer storage.Writer,
+	rangeID roachpb.RangeID,
+	keys roachpb.RSpan,
+) error {
+	return clearRangeData(ctx, rangeID, reader, writer, clearRangeDataOptions{
+		// Since the RHS replica is uninitalized, we know there isn't anything in
+		// the two replicated spans below, before the current batch. Setting these
+		// options will in effect only clear the writes to the RHS replicated state
+		// staged in the batch.
+		clearReplicatedBySpan:    keys,
+		clearReplicatedByRangeID: true,
+		// TODO(tbg): we don't actually want to touch the raft state of the RHS
+		// replica since it's absent or a more recent one than in the split. Now
+		// that we have a bool targeting unreplicated RangeID-local keys, we can set
+		// it to false and remove the HardState+ReplicaID write-back in the caller.
+		// However, there can be historical split proposals with the
+		// RaftTruncatedState key set in splitTriggerHelper[^1]. We must first make
+		// sure that such proposals no longer exist, e.g. with a below-raft
+		// migration.
+		//
+		// [^1]: https://github.com/cockroachdb/cockroach/blob/f263a765d750e41f2701da0a923a6e92d09159fa/pkg/kv/kvserver/batcheval/cmd_end_transaction.go#L1109-L1149
+		//
+		// See also: https://github.com/cockroachdb/cockroach/issues/94933
+		clearUnreplicatedByRangeID: true,
+	})
+}

pkg/kv/kvserver/kvstorage/destroy_test.go

@@ -69,13 +69,7 @@ func TestDestroyReplica(t *testing.T) {
 	})
 	mutate("destroy", func(rw storage.ReadWriter) {
 		require.NoError(t, DestroyReplica(
-			ctx, rw, rw,
-			DestroyReplicaInfo{FullReplicaID: r.id, Keys: r.keys}, r.id.ReplicaID+1,
-			ClearRangeDataOptions{
-				ClearUnreplicatedByRangeID: true,
-				ClearReplicatedByRangeID:   true,
-				ClearReplicatedBySpan:      r.keys,
-			},
+			ctx, rw, rw, DestroyReplicaInfo{FullReplicaID: r.id, Keys: r.keys}, r.id.ReplicaID+1,
 		))
 	})
 

pkg/kv/kvserver/replica_app_batch.go

@@ -359,21 +359,10 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		rhsRepl.mu.Unlock()
 		rhsRepl.readOnlyCmdMu.Unlock()
 
-		// Use math.MaxInt32 (mergedTombstoneReplicaID) as the nextReplicaID as an
-		// extra safeguard against creating new replicas of the RHS. This isn't
-		// required for correctness, since the merge protocol should guarantee that
-		// no new replicas of the RHS can ever be created, but it doesn't hurt to
-		// be careful.
-		if err := kvstorage.DestroyReplica(
-			ctx, b.batch, b.batch,
-			rhsRepl.destroyInfoRaftMuLocked(),
-			mergedTombstoneReplicaID,
-			kvstorage.ClearRangeDataOptions{
-				ClearReplicatedByRangeID:   true,
-				ClearUnreplicatedByRangeID: true,
-			},
+		if _, err := kvstorage.SubsumeReplica(
+			ctx, b.batch, b.batch, rhsRepl.destroyInfoRaftMuLocked(), false, /* forceSortedKeys */
 		); err != nil {
-			return errors.Wrapf(err, "unable to destroy replica before merge")
+			return errors.Wrapf(err, "unable to subsume replica before merge")
 		}
 
 		// Shut down rangefeed processors on either side of the merge.
@@ -451,7 +440,6 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		b.r.shMu.destroyStatus.Set(
 			kvpb.NewRangeNotFoundError(b.r.RangeID, b.r.store.StoreID()),
 			destroyReasonRemoved)
-		span := b.r.descRLocked().RSpan()
 		b.r.mu.Unlock()
 		b.r.readOnlyCmdMu.Unlock()
 		b.changeRemovesReplica = true
@@ -461,14 +449,7 @@ func (b *replicaAppBatch) runPostAddTriggersReplicaOnly(
 		// above, and DestroyReplica will also add a range tombstone to the
 		// batch, so that when we commit it, the removal is finalized.
 		if err := kvstorage.DestroyReplica(
-			ctx, b.batch, b.batch,
-			b.r.destroyInfoRaftMuLocked(),
-			change.NextReplicaID(),
-			kvstorage.ClearRangeDataOptions{
-				ClearReplicatedBySpan:      span,
-				ClearReplicatedByRangeID:   true,
-				ClearUnreplicatedByRangeID: true,
-			},
+			ctx, b.batch, b.batch, b.r.destroyInfoRaftMuLocked(), change.NextReplicaID(),
 		); err != nil {
 			return errors.Wrapf(err, "unable to destroy replica before removal")
 		}

pkg/kv/kvserver/replica_destroy.go

@@ -8,7 +8,6 @@ package kvserver
 import (
 	"context"
 	"fmt"
-	"math"
 
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/apply"
@@ -56,14 +55,6 @@ func (s destroyStatus) Removed() bool {
 	return s.reason == destroyReasonRemoved
 }
 
-// mergedTombstoneReplicaID is the replica ID written into the tombstone
-// for replicas which are part of a range which is known to have been merged.
-// This value should prevent any messages from stale replicas of that range from
-// ever resurrecting merged replicas. Whenever merging or subsuming a replica we
-// know new replicas can never be created so this value is used even if we
-// don't know the current replica ID.
-const mergedTombstoneReplicaID roachpb.ReplicaID = math.MaxInt32
-
 // postDestroyRaftMuLocked is called after the replica destruction is durably
 // written to Pebble.
 func (r *Replica) postDestroyRaftMuLocked(ctx context.Context) error {
@@ -97,23 +88,10 @@ func (r *Replica) destroyRaftMuLocked(ctx context.Context, nextReplicaID roachpb
 	ms := r.GetMVCCStats()
 	batch := r.store.TODOEngine().NewWriteBatch()
 	defer batch.Close()
-	desc := r.Desc()
-	inited := desc.IsInitialized()
-
-	opts := kvstorage.ClearRangeDataOptions{
-		ClearReplicatedBySpan: desc.RSpan(), // zero if !inited
-		// TODO(tbg): if it's uninitialized, we might as well clear
-		// the replicated state because there isn't any. This seems
-		// like it would be simpler, but needs a code audit to ensure
-		// callers don't call this in in-between states where the above
-		// assumption doesn't hold.
-		ClearReplicatedByRangeID:   inited,
-		ClearUnreplicatedByRangeID: true,
-	}
+
 	// TODO(sep-raft-log): need both engines separately here.
 	if err := kvstorage.DestroyReplica(
-		ctx, r.store.TODOEngine(), batch,
-		r.destroyInfoRaftMuLocked(), nextReplicaID, opts,
+		ctx, r.store.TODOEngine(), batch, r.destroyInfoRaftMuLocked(), nextReplicaID,
 	); err != nil {
 		return err
 	}

pkg/kv/kvserver/replica_gc_queue.go

@@ -12,6 +12,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb"
 	"github.com/cockroachdb/cockroach/pkg/raft/raftpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -348,11 +349,11 @@ func (rgcq *replicaGCQueue) process(
 			}
 		}
 
-		// A tombstone is written with a value of mergedTombstoneReplicaID because
+		// A tombstone is written with a value of MergedTombstoneReplicaID because
 		// we know the range to have been merged. See the Merge case of
 		// runPreApplyTriggers() for details.
 		if err := repl.store.RemoveReplica(
-			ctx, repl, mergedTombstoneReplicaID, "dangling subsume via replica GC queue",
+			ctx, repl, kvstorage.MergedTombstoneReplicaID, "dangling subsume via replica GC queue",
 		); err != nil {
 			return false, err
 		}

pkg/kv/kvserver/replica_raftstorage.go

@@ -818,7 +818,7 @@ func (r *Replica) clearSubsumedReplicaInMemoryData(
 		// allowed in (perhaps not involving any of the RangeIDs known to the merge
 		// but still touching its keyspace) and causing corruption.
 		ph, err := r.store.removeInitializedReplicaRaftMuLocked(
-			ctx, sr, mergedTombstoneReplicaID, "subsumed by snapshot",
+			ctx, sr, kvstorage.MergedTombstoneReplicaID, "subsumed by snapshot",
 			RemoveOptions{
 				// The data was already destroyed by clearSubsumedReplicaDiskData.
 				DestroyData:       false,

pkg/kv/kvserver/snapshot_apply_prepare.go

@@ -150,20 +150,9 @@ 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 {
-			// NOTE: We set mustClearRange to true because we are setting
-			// RangeTombstoneKey. Since Clears and Puts need to be done in increasing
-			// order of keys, it is not safe to use ClearRangeIter.
-			opts := kvstorage.ClearRangeDataOptions{
-				ClearReplicatedByRangeID:   true,
-				ClearUnreplicatedByRangeID: true,
-				MustUseClearRange:          true,
-			}
-			s.cleared = append(s.cleared, rditer.Select(sub.RangeID, rditer.SelectOpts{
-				ReplicatedByRangeID:   opts.ClearReplicatedByRangeID,
-				UnreplicatedByRangeID: opts.ClearUnreplicatedByRangeID,
-			})...)
-			// NB: Actually clear RangeID local key spans.
-			return kvstorage.DestroyReplica(ctx, reader, w, sub, mergedTombstoneReplicaID, opts)
+			opts, err := kvstorage.SubsumeReplica(ctx, reader, w, sub, true /* forceSortedKeys */)
+			s.cleared = append(s.cleared, rditer.Select(sub.RangeID, opts)...)
+			return err
 		}); err != nil {
 			return err
 		}