Merge #142653 #144215

142653: kvserver, storage: clean up, fix bugs in, and test snapshot receive path r=tbg a=tbg

When a snapshot is received, we write the incoming KV pairs into a number of SSTs. With the introduction of excise and shared/external SSTs (disagg storage), this code had become quite crufty and supported - in principle - multiple modes that made it quite brittle and under-tested.

In this PR, we "bake in" excises: for the SST covering the MVCC span, we don't lay down a range deletion but instead *always* use excise. We now also always allow splitting of large MVCC SSTs (into reasonably-sized SSTs). This works against the previous proliferation of options, and enabled various code improvements.

The code around range keys (MVCC rangedels) was cleaned up too, and missing fragmentation was added. For "regular" rangedels (ClearRange), fragmentation was missing entirely and was added as well.

We now have a datadriven unit test for the MultiSSTWriter, which shows precisely what's contained in each produced SST. This doesn't mean the receive path is well-tested - far from it - but at least there's a solid foundation to build on top of.

Overall, the code here is much improved and easier to reason about.

Epic: CRDB-46488
Release Note: None

144215: vecindex: update vector index key and value encodings r=drewkimball a=andy-kimball

First commit:
When multiple workers are racing to create the root partition, it's possible to trigger an error when the root partition already exists and is in a state other than Ready (e.g. Splitting). Fix this by creating the root metadata record only if doesn't already exist. If it does exist, just return the current metadata.

Second commit:
Make two updates to how KV keys and values are encoded for vector indexes:

1. Add Family ID to the end of the metadata KV key encoding. This is required by the KV split code, which checks the family ID to ensure that splits don't split column families for the same row across ranges.

2. Encode the partition state fields in the metadata KV value. This is needed for the new non-transactional background split code.

Epic: CRDB-42943

Release note: None

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

Author: 3 people

pkg/kv/kvserver/BUILD.bazel

@@ -254,6 +254,7 @@ go_library(
         "@com_github_cockroachdb_pebble//:pebble",
         "@com_github_cockroachdb_pebble//objstorage",
         "@com_github_cockroachdb_pebble//objstorage/remote",
+        "@com_github_cockroachdb_pebble//rangedel",
         "@com_github_cockroachdb_pebble//rangekey",
         "@com_github_cockroachdb_pebble//sstable/block",
         "@com_github_cockroachdb_pebble//vfs",
@@ -564,6 +565,7 @@ go_test(
         "@com_github_cockroachdb_errors//oserror",
         "@com_github_cockroachdb_logtags//:logtags",
         "@com_github_cockroachdb_pebble//:pebble",
+        "@com_github_cockroachdb_pebble//rangekey",
         "@com_github_cockroachdb_pebble//sstable",
         "@com_github_cockroachdb_pebble//vfs",
         "@com_github_cockroachdb_redact//:redact",

pkg/kv/kvserver/client_merge_test.go

@@ -3905,11 +3905,15 @@ func TestStoreRangeMergeRaftSnapshot(t *testing.T) {
 		}
 		keySpans := rditer.MakeReplicatedKeySpans(inSnap.Desc)
 		sstFileWriters := map[string]sstFileWriter{}
-		for _, span := range keySpans {
+		for i, span := range keySpans {
 			file := &storage.MemObject{}
 			writer := storage.MakeIngestionSSTWriter(ctx, st, file)
-			if err := writer.ClearRawRange(span.Key, span.EndKey, true /* pointKeys */, true /* rangeKeys */); err != nil {
-				return err
+			if i < len(keySpans)-1 {
+				// The last span is the MVCC span, and is always cleared via Excise.
+				// See multiSSTWriter.
+				if err := writer.ClearRawRange(span.Key, span.EndKey, true /* pointKeys */, true /* rangeKeys */); err != nil {
+					return err
+				}
 			}
 			sstFileWriters[string(span.Key)] = sstFileWriter{
 				span:   span,

pkg/kv/kvserver/kv_snapshot_strategy.go

@@ -115,41 +115,10 @@ func (kvSS *kvBatchSnapshotStrategy) Receive(
 		return noSnap, errors.AssertionFailedf("last span in multiSSTWriter did not equal the user key span: %s", keyRanges[len(keyRanges)-1].String())
 	}
 
-	// TODO(aaditya): Remove once we support flushableIngests for shared and
-	// external files in the engine.
-
-	// TODO(tbg): skipClearForMVCCSpan should equal true (since we always excise
-	// the MVCC span), but there appear to be bugs in this code as demonstrated by
-	// TestRaftSnapshotsWithMVCCRangeKeysEverywhere failing with the proposed
-	// change:
-	//
-	// * panic: failed to put range key in sst: pebble: spans must be added in order: /Local/RangeID/75/r":a"/0,0 > /Local/RangeID/75/r""/0,0
-	// ^- we added the ":a" key first, now we're trying to add the r"" key.
-	//
-	// My basic understanding of this failure mode is:
-	// - first, a range del covering the range-local replicated key span `/Range/75/{r-s}` is added in (msstw.initSST)
-	// - the fragmenter's start key is now `/Range/75/r`.
-	// - when the `/Range/75/r/{:a-:z}` rangedel, is handled, it enters through `msstw.PutRangeKey`
-	//    - since  `skipClearForMVCCSpan` is set, this does not update the fragmenter.
-	//    - the range deletion is added to the current SST.
-	// - when the first key >= `/Range/75/r` is added, `msstw.finalizeSST` flushes
-	//   the fragmenter to the current SST.
-	// - we hit the above error, since the SST already contains the rangedel
-	//   starting at `/Range/75/r:a` and we're not attempting to add a rangedel
-	//   `/Range/75/r` with smaller start key.
-	//
-	// The crucial problem is skipping the fragmenter. This seems to have to do with
-	// wanting to allow callers to use `skipClearForMVCCSpan`, but this is going to
-	// be dead code.
-	//
-	// Another note: CI passes when this is unconditionally set to `false`,
-	// indicating that we have poor coverage of the shared and external code paths.
-	skipClearForMVCCSpan := header.SharedReplicate || header.ExternalReplicate
-
 	// 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, skipClearForMVCCSpan, header.RangeKeysInOrder)
+	msstw, err := newMultiSSTWriter(ctx, kvSS.st, kvSS.scratch, localRanges, mvccRange, kvSS.sstChunkSize, header.RangeKeysInOrder)
 	if err != nil {
 		return noSnap, err
 	}
@@ -294,19 +263,18 @@ func (kvSS *kvBatchSnapshotStrategy) Receive(
 			}
 
 			inSnap := IncomingSnapshot{
-				SnapUUID:                    snapUUID,
-				SSTStorageScratch:           kvSS.scratch,
-				FromReplica:                 header.RaftMessageRequest.FromReplica,
-				Desc:                        header.State.Desc,
-				DataSize:                    dataSize,
-				SSTSize:                     sstSize,
-				SharedSize:                  sharedSize,
-				raftAppliedIndex:            header.State.RaftAppliedIndex,
-				msgAppRespCh:                make(chan raftpb.Message, 1),
-				sharedSSTs:                  sharedSSTs,
-				externalSSTs:                externalSSTs,
-				includesRangeDelForLastSpan: !skipClearForMVCCSpan,
-				clearedSpans:                keyRanges,
+				SnapUUID:          snapUUID,
+				SSTStorageScratch: kvSS.scratch,
+				FromReplica:       header.RaftMessageRequest.FromReplica,
+				Desc:              header.State.Desc,
+				DataSize:          dataSize,
+				SSTSize:           sstSize,
+				SharedSize:        sharedSize,
+				raftAppliedIndex:  header.State.RaftAppliedIndex,
+				msgAppRespCh:      make(chan raftpb.Message, 1),
+				sharedSSTs:        sharedSSTs,
+				externalSSTs:      externalSSTs,
+				clearedSpans:      keyRanges,
 			}
 
 			timingTag.stop("totalTime")