Merge #155847

155847: keys: assert that schema is immutable r=arulajmani,iskettaneh a=pav-kv

This PR adds a test which ensures that storage schema in `doc.go` does not change. If it does, the test fails and forces the author to think harder about the implications of the change.

**Context / example of a concern**: in replica destruction code (`kvstorage.DestroyReplica` and variants), we should destroy, among other things, the “unreplicated” keys. With separated engines this is tricky because the raft and state machine keys are inconveniently interleaved. Instead of using various ranged deletions that cover the entire keyspace, we can manually clear each key (which we all know). But if we do so, there is a risk someone adds a new key to the unreplicated section and forgets to update the destruction code. This at best could lead to a small disk space leak, and at worst a more serious bug in replica lifecycle.

**Another example**: the unreplicated space does not have ranged Pebble keys. Some code relies on this, e.g. `applySnapshot` does not clear ranged keys when rewriting the replica state (as an optimization, because there are none). It will have to start doing so if ranged keys are ever added to the unreplicated span.

Related to #152845

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

Author: craig[bot]

pkg/keys/BUILD.bazel

@@ -29,6 +29,7 @@ go_test(
     name = "keys_test",
     size = "small",
     srcs = [
+        "doc_test.go",
         "keys_test.go",
         "printer_test.go",
         "sql_test.go",

pkg/keys/doc.go

@@ -146,7 +146,9 @@ package keys
 
 // NB: The sorting order of the symbols below map to the physical layout.
 // Preserve group-wise ordering when adding new constants.
-var _ = [...]interface{}{
+//
+// See TestSchemaIsStable for additional considerations when changing schema.
+var schema = [...]interface{}{
 	MinKey,
 
 	// There are five types of local key data enumerated below: replicated
@@ -176,19 +178,21 @@ var _ = [...]interface{}{
 	//   range as a whole. Though they are replicated, they are unaddressable.
 	//   Typical examples are MVCC stats and the abort span. They all share
 	//   `LocalRangeIDPrefix` and `LocalRangeIDReplicatedInfix`.
-	AbortSpanKey,             // "abc-"
-	RangeGCThresholdKey,      // "lgc-"
-	RangeAppliedStateKey,     // "rask"
-	RangeForceFlushKey,       // "rffk"
-	RangeLeaseKey,            // "rll-"
-	RangePriorReadSummaryKey, // "rprs"
+	LocalRangeIDReplicatedInfix,                 // "r"
+	AbortSpanKey,                                // "abc-"
+	RangeGCThresholdKey,                         // "lgc-"
+	RangeAppliedStateKey,                        // "rask"
+	RangeForceFlushKey,                          // "rffk"
+	RangeLeaseKey,                               // "rll-"
+	RangePriorReadSummaryKey,                    // "rprs"
 	ReplicatedSharedLocksTransactionLatchingKey, // "rsl-"
-	RangeVersionKey, // "rver"
+	RangeVersionKey,                             // "rver"
 
 	//   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`.
+	localRangeIDUnreplicatedInfix,  // "u"
 	RangeTombstoneKey,              // "rftb"
 	RaftHardStateKey,               // "rfth"
 	RaftLogKey,                     // "rftl"
@@ -200,6 +204,7 @@ var _ = [...]interface{}{
 	//   as a whole. They are replicated and addressable. Typical examples are
 	//   the range descriptor and transaction records. They all share
 	//   `LocalRangePrefix`.
+	LocalRangePrefix,      // "k"
 	RangeProbeKey,         // "prbe"
 	QueueLastProcessedKey, // "qlpt"
 	RangeDescriptorKey,    // "rdsc"
@@ -208,6 +213,7 @@ var _ = [...]interface{}{
 	//   4. Store local keys: These contain metadata about an individual store.
 	//   They are unreplicated and unaddressable. The typical example is the
 	//   store 'ident' record. They all share `localStorePrefix`.
+	LocalStorePrefix,                 // "s"
 	DeprecatedStoreClusterVersionKey, // "cver"
 	StoreGossipKey,                   // "goss"
 	StoreHLCUpperBoundKey,            // "hlcu"
@@ -229,6 +235,7 @@ var _ = [...]interface{}{
 	//   Single key locks use a byte, LockTableSingleKeyInfix, that follows
 	//   the LocalRangeLockTablePrefix. This is to keep the single-key locks
 	//   separate from (future) range locks.
+	LocalRangeLockTablePrefix, // "z"
 	LockTableSingleKey,
 
 	// The global keyspace includes the meta{1,2}, system, system tenant SQL

pkg/keys/doc_test.go

@@ -0,0 +1,92 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package keys
+
+import (
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/stretchr/testify/require"
+)
+
+// TestSchemaIsStable asserts that the storage schema in doc.go does not change.
+// If it does (e.g. there is a new key), this test fails and nudges the author
+// to think through the implications.
+//
+// When changing the schema, it is a good idea to read the comments in doc.go
+// around the modified sections, and understand the lifecycle of the relevant
+// keys. For example, look at the code where the related keys are read, written,
+// modified, and removed.
+//
+// Note that it can be tricky: e.g. some code might be clearing a whole span of
+// keys (like unreplicated RangeID-local space), without explicitly calling out
+// individual keys, so the author should be aware of this to understand where
+// the lifetime of their new key ends.
+//
+// Conversely, some key spans can be cleared key-by-key, for efficiency or
+// simplicity reasons. New keys might need to be explicitly cleared in those
+// places, so that there is no storage space leakage. The author should not
+// assume that keys are cleared auto-magically in the right places. There are
+// "gaps" in the keyspace that don't have proper management because the code
+// assumes they are always empty.
+//
+// Another important aspect to consider is version compatibility, i.e. how
+// adjacent CRDB versions handle the added or removed keys. It might be
+// necessary to complete a migration to make a transition safe.
+//
+// Once this due diligence is done, it is ok to trivially fix this test.
+//
+// TODO(pav-kv): make a proper "structured" schema instead of a flat list of
+// things, and make the assertions stronger (currently they only count "lines"
+// in the schema).
+func TestSchemaIsStable(t *testing.T) {
+	pos := make(map[string]struct {
+		first int
+		last  int
+	})
+	add := func(k string, v int) {
+		p, found := pos[k]
+		if found {
+			p.last++
+			require.Equal(t, p.last, v)
+		} else {
+			p.first = v
+			p.last = v
+		}
+		pos[k] = p
+	}
+	for i, ref := range schema {
+		switch ref := ref.(type) {
+		case []byte:
+			add(string(ref), i)
+		case roachpb.Key:
+			add(string(ref), i)
+		}
+	}
+
+	section := func(from, to string, items int) {
+		t.Helper()
+		fromPos, ok := pos[from]
+		require.True(t, ok)
+		toPos, ok := pos[to]
+		require.True(t, ok)
+		require.Greater(t, toPos.first, fromPos.last)
+		require.Equal(t, items, toPos.first-fromPos.last-1)
+	}
+
+	// The replicated RangeID-local keys section.
+	// WARNING: if this line fails, don't just fix it, read the test comment.
+	section(string(LocalRangeIDReplicatedInfix), string(localRangeIDUnreplicatedInfix), 8)
+	// The unreplicated RangeID-local keys section.
+	// WARNING: if this line fails, don't just fix it, read the test comment.
+	section(string(localRangeIDUnreplicatedInfix), string(LocalRangePrefix), 6)
+	// The replicated range-local keys section.
+	// WARNING: if this line fails, don't just fix it, read the test comment.
+	section(string(LocalRangePrefix), string(LocalStorePrefix), 4)
+	// The store-local keys section.
+	// WARNING: if this line fails, don't just fix it, read the test comment.
+	section(string(LocalStorePrefix), string(LocalRangeLockTablePrefix), 12)
+}