Merge #144613

144613: kvserver: hard-code RangeKeysInOrder=true r=tbg a=tbg

In the 24.3 release, this bool was introduced and unconditionally set to true in #129088:

    $ git log v24.3.0 -1 --oneline --grep 'kvserver: reenable splitting of snapshot sstables'
    129088: kvserver: reenable splitting of snapshot sstables r=aadityasondhi a=itsbilal

    This change updates the snapshot strategy's sender side to iterate over
    points and ranges together, instead of only iterating on points first, then
    only ranges. This allows us to more efficiently split snapshot sstables on
    the receiver side. To avoid the need to add a version gate on the receiver
    side, we propagate a bool, RangeKeysInOrder, to the receiver which is a
    signal to it to enable sstable splits.

Since we are now in the v25.3 release cycle, we can safely assume that this bool is always
set on all incoming snapshots headers. This is because the v25.2 release is not skippable
and both possible upgrade paths into it go through v24.3, which already sets this bool. 
The same will be true for the v25.3 release, which can only be reached through v25.2.

![image](https://github.com/user-attachments/assets/04aa9dd3-7e95-495f-ae83-cfdef4c3ed6f)

*However*, we cannot remove the header field yet. This can only be done in a
follow-up migration in a few releases, since the absence of the field would
communicate to receivers who are not running with this first step of the
migration (25.2 and before) that the range keys are out of order, but they
would not be. Perhaps the code would support this, but it's not a risk we're
interested in taking.

We can only remove the proto field in the v26.1 release: v26.1 is the first
release that has 25.4 in its required upgrade path, and since 25.3 is skippable,
25.4 still has to interop with 25.2 which precedes the hard-coding of the bool.
A comment to this effect was added on the proto.

Epic: CRDB-46488
Release note: none
 

Co-authored-by: Tobias Grieger <[email protected]>

Author: craig[bot]

pkg/kv/kvserver/kv_snapshot_strategy.go

@@ -118,7 +118,7 @@ func (kvSS *kvBatchSnapshotStrategy) Receive(
 	// The last key range is the user key span.
 	localRanges := keyRanges[:len(keyRanges)-1]
 	mvccRange := keyRanges[len(keyRanges)-1]
-	msstw, err := newMultiSSTWriter(ctx, kvSS.st, kvSS.scratch, localRanges, mvccRange, kvSS.sstChunkSize, header.RangeKeysInOrder)
+	msstw, err := newMultiSSTWriter(ctx, kvSS.st, kvSS.scratch, localRanges, mvccRange, kvSS.sstChunkSize)
 	if err != nil {
 		return noSnap, err
 	}

pkg/kv/kvserver/kvserverpb/raft.proto

@@ -244,6 +244,12 @@ message SnapshotRequest {
     // keys in key order, as opposed to iterating over point keys first
     // and range keys second. The receiver can take advantage of this
     // to split points/range keys into multiple sstables for ingestion.
+    //
+    // MIGRATION: v24.3 and newer will always set this field to true, and v25.3
+    // and newer stop consulting the field altogether (assume it is true). In
+    // v26.1, we can then drop this field altogether, since v26.1 is the first
+    // version that does not have to interop with v25.2 (the last version to
+    // attach meaning to this field being unset/absent) or older.
     bool range_keys_in_order = 14;
 
     reserved 1, 4, 6, 7, 8, 9;

pkg/kv/kvserver/multi_sst_writer.go

@@ -70,7 +70,6 @@ func newMultiSSTWriter(
 	localKeySpans []roachpb.Span,
 	mvccKeySpan roachpb.Span,
 	sstChunkSize int64,
-	rangeKeysInOrder bool,
 ) (*multiSSTWriter, error) {
 	msstw := &multiSSTWriter{
 		st:            st,
@@ -82,15 +81,7 @@ func newMultiSSTWriter(
 			End:   storage.EngineKey{Key: mvccKeySpan.EndKey},
 		}},
 		sstChunkSize: sstChunkSize,
-	}
-	if rangeKeysInOrder {
-		// We disable snapshot sstable splitting unless the sender has
-		// specified in its snapshot header that it is sending range keys in
-		// key order alongside point keys, as opposed to sending them at the end
-		// of the snapshot. This is necessary to efficiently produce fragmented
-		// snapshot sstables, as otherwise range keys will arrive out-of-order
-		// wrt. point keys.
-		msstw.maxSSTSize = MaxSnapshotSSTableSize.Get(&st.SV)
+		maxSSTSize:   MaxSnapshotSSTableSize.Get(&st.SV),
 	}
 	msstw.rangeKeyFrag = rangekey.Fragmenter{
 		Cmp:    storage.EngineComparer.Compare,

pkg/kv/kvserver/replica_sst_snapshot_storage_test.go

@@ -298,10 +298,7 @@ func testMultiSSTWriterInitSSTInner(t *testing.T, interesting bool) {
 	// Disabling columnar blocks causes stats changes.
 	storage.ColumnarBlocksEnabled.Override(context.Background(), &st.SV, true)
 
-	msstw, err := newMultiSSTWriter(
-		ctx, st, scratch, localSpans, mvccSpan, 0,
-		false, /* rangeKeysInOrder */
-	)
+	msstw, err := newMultiSSTWriter(ctx, st, scratch, localSpans, mvccSpan, 0)
 	require.NoError(t, err)
 
 	var buf redact.StringBuilder
@@ -430,7 +427,7 @@ func TestMultiSSTWriterSize(t *testing.T) {
 	mvccSpan := keySpans[len(keySpans)-1]
 
 	// Make a reference msstw with the default size.
-	referenceMsstw, err := newMultiSSTWriter(ctx, settings, ref, localSpans, mvccSpan, 0, true /* rangeKeysInOrder */)
+	referenceMsstw, err := newMultiSSTWriter(ctx, settings, ref, localSpans, mvccSpan, 0)
 	require.NoError(t, err)
 	require.Equal(t, int64(0), referenceMsstw.dataSize)
 	now := timeutil.Now().UnixNano()
@@ -462,7 +459,7 @@ func TestMultiSSTWriterSize(t *testing.T) {
 
 	MaxSnapshotSSTableSize.Override(ctx, &settings.SV, 100)
 
-	multiSSTWriter, err := newMultiSSTWriter(ctx, settings, scratch, localSpans, mvccSpan, 0, true)
+	multiSSTWriter, err := newMultiSSTWriter(ctx, settings, scratch, localSpans, mvccSpan, 0)
 	require.NoError(t, err)
 	require.Equal(t, int64(0), multiSSTWriter.dataSize)
 
@@ -544,7 +541,7 @@ func TestMultiSSTWriterAddLastSpan(t *testing.T) {
 	localSpans := keySpans[:len(keySpans)-1]
 	mvccSpan := keySpans[len(keySpans)-1]
 
-	msstw, err := newMultiSSTWriter(ctx, cluster.MakeTestingClusterSettings(), scratch, localSpans, mvccSpan, 0, false)
+	msstw, err := newMultiSSTWriter(ctx, cluster.MakeTestingClusterSettings(), scratch, localSpans, mvccSpan, 0)
 	require.NoError(t, err)
 	testKey := storage.MVCCKey{Key: roachpb.RKey("d1").AsRawKey(), Timestamp: hlc.Timestamp{WallTime: 1}}
 	testEngineKey, _ := storage.DecodeEngineKey(storage.EncodeMVCCKey(testKey))