Merge #149620 #149695

149620: kvserver: do not write truncated state during evaluation r=pav-kv a=arulajmani

This moves writing the truncated state from evaluation to splitPreApply, which is run locally on each store before the write batch constructed as part of the split is applied. This means we no longer replicate state that nominally belongs to the raft engine.

Closes #144540

Release note: None

149695: catalog: make DescriptorType unredactable r=rafiss a=rafiss

fixes #149396
Release note: None

Co-authored-by: Arul Ajmani <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

Author: 3 people

docs/generated/settings/settings-for-tenants.txt

@@ -415,4 +415,4 @@ trace.zipkin.collector	string		the address of a Zipkin instance to receive trace
 ui.database_locality_metadata.enabled	boolean	true	if enabled shows extended locality data about databases and tables in DB Console which can be expensive to compute	application
 ui.default_timezone	string		the default timezone used to format timestamps in the ui	application
 ui.display_timezone	enumeration	etc/utc	the timezone used to format timestamps in the ui. This setting is deprecatedand will be removed in a future version. Use the 'ui.default_timezone' setting instead. 'ui.default_timezone' takes precedence over this setting. [etc/utc = 0, america/new_york = 1]	application
-version	version	1000025.2-upgrading-to-1000025.3-step-008	set the active cluster version in the format '<major>.<minor>'	application
+version	version	1000025.2-upgrading-to-1000025.3-step-010	set the active cluster version in the format '<major>.<minor>'	application

docs/generated/settings/settings.html

@@ -373,6 +373,6 @@
 <tr><td><div id="setting-ui-database-locality-metadata-enabled" class="anchored"><code>ui.database_locality_metadata.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>if enabled shows extended locality data about databases and tables in DB Console which can be expensive to compute</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-ui-default-timezone" class="anchored"><code>ui.default_timezone</code></div></td><td>string</td><td><code></code></td><td>the default timezone used to format timestamps in the ui</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-ui-display-timezone" class="anchored"><code>ui.display_timezone</code></div></td><td>enumeration</td><td><code>etc/utc</code></td><td>the timezone used to format timestamps in the ui. This setting is deprecatedand will be removed in a future version. Use the &#39;ui.default_timezone&#39; setting instead. &#39;ui.default_timezone&#39; takes precedence over this setting. [etc/utc = 0, america/new_york = 1]</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000025.2-upgrading-to-1000025.3-step-008</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-version" class="anchored"><code>version</code></div></td><td>version</td><td><code>1000025.2-upgrading-to-1000025.3-step-010</code></td><td>set the active cluster version in the format &#39;&lt;major&gt;.&lt;minor&gt;&#39;</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 </tbody>
 </table>

pkg/clusterversion/cockroach_versions.go

@@ -219,6 +219,13 @@ const (
 	V25_3_AddEstimatedLastLoginTime
 
 	V25_3_AddHotRangeLoggerJob
+
+	// V25_3_WriteInitialTruncStateBeforeSplitApplication is the version above
+	// which we write the initial truncated state before applying a split. By
+	// extension, we no longer need to replicate the truncated state when
+	// constructing the split write batch.
+	V25_3_WriteInitialTruncStateBeforeSplitApplication
+
 	// *************************************************
 	// Step (1) Add new versions above this comment.
 	// Do not add new versions to a patch release.
@@ -279,6 +286,8 @@ var versionTable = [numKeys]roachpb.Version{
 
 	V25_3_AddHotRangeLoggerJob: {Major: 25, Minor: 2, Internal: 8},
 
+	V25_3_WriteInitialTruncStateBeforeSplitApplication: {Major: 25, Minor: 2, Internal: 10},
+
 	// *************************************************
 	// Step (2): Add new versions above this comment.
 	// Do not add new versions to a patch release.

pkg/kv/kvserver/BUILD.bazel

@@ -579,6 +579,7 @@ go_test(
         "@org_golang_google_grpc//:grpc",
         "@org_golang_google_grpc//metadata",
         "@org_golang_x_exp//maps",
+        "@org_golang_x_exp//slices",
         "@org_golang_x_sync//errgroup",
         "@org_golang_x_sync//syncmap",
     ],

pkg/kv/kvserver/batcheval/cmd_end_transaction.go

@@ -1481,9 +1481,15 @@ func splitTriggerHelper(
 		); err != nil {
 			return enginepb.MVCCStats{}, result.Result{}, errors.Wrap(err, "unable to write initial Replica state")
 		}
-		// TODO(arulajmani): remove WriteInitialTruncState.
-		if err := stateloader.WriteInitialTruncState(ctx, batch, split.RightDesc.RangeID); err != nil {
-			return enginepb.MVCCStats{}, result.Result{}, errors.Wrap(err, "unable to write initial Replica state")
+		// TODO(arulajmani): This can be removed once all nodes are past the
+		// V25_3_WriteInitialTruncStateBeforeSplitApplication cluster version.
+		// At that point, we'll no longer need to replicate the truncated state
+		// as all replicas will be responsible for writing it locally before
+		// applying the split.
+		if !rec.ClusterSettings().Version.IsActive(ctx, clusterversion.V25_3_WriteInitialTruncStateBeforeSplitApplication) {
+			if err := stateloader.WriteInitialTruncState(ctx, batch, split.RightDesc.RangeID); err != nil {
+				return enginepb.MVCCStats{}, result.Result{}, errors.Wrap(err, "unable to write initial Replica state")
+			}
 		}
 	}
 

pkg/kv/kvserver/batcheval/cmd_end_transaction_test.go

@@ -2232,13 +2232,14 @@ func TestSplitTriggerWritesInitialReplicaState(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, loadedGCHint)
 	require.Equal(t, gcHint, *loadedGCHint)
-	expTruncState := kvserverpb.RaftTruncatedState{
-		Term:  stateloader.RaftInitialLogTerm,
-		Index: stateloader.RaftInitialLogIndex,
-	}
+
+	// The split trigger doesn't write the initial truncated state for the RHS
+	// as it isn't part of the range's applied state.
+	expTruncState := kvserverpb.RaftTruncatedState{}
 	loadedTruncState, err := slRight.LoadRaftTruncatedState(ctx, batch)
 	require.NoError(t, err)
 	require.Equal(t, expTruncState, loadedTruncState)
+
 	loadedVersion, err := slRight.LoadVersion(ctx, batch)
 	require.NoError(t, err)
 	require.Equal(t, version, loadedVersion)

pkg/kv/kvserver/stateloader/initial.go

@@ -107,8 +107,10 @@ func WriteInitialReplicaState(
 	return newMS, nil
 }
 
-// WriteInitialTruncState writes the initial RaftTruncatedState.
-// TODO(arulajmani): remove this.
+// WriteInitialTruncState writes the initial truncated state.
+//
+// TODO(arul): this can be removed once no longer call this from the split
+// evaluation path.
 func WriteInitialTruncState(ctx context.Context, w storage.Writer, rangeID roachpb.RangeID) error {
 	return logstore.NewStateLoader(rangeID).SetRaftTruncatedState(ctx, w,
 		&kvserverpb.RaftTruncatedState{

pkg/kv/kvserver/store_split.go

@@ -9,6 +9,7 @@ import (
 	"bytes"
 	"context"
 
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/load"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/stateloader"
@@ -131,13 +132,24 @@ func splitPreApply(
 	}
 
 	// The RHS replica exists and is uninitialized. We are initializing it here.
-	// Update the raft HardState with the new Commit index (taken from the applied
-	// state in the write batch), and use existing or default Term and Vote. This
-	// is the common case.
+	// This is the common case.
+	//
+	// Update the raft HardState with the new Commit index (taken from the
+	// applied state in the write batch), and use existing[*] or default Term
+	// and Vote. Also write the initial RaftTruncatedState.
+	//
+	// [*] Note that uninitialized replicas may cast votes, and if they have, we
+	// can't load the default Term and Vote values.
 	rsl := stateloader.Make(split.RightDesc.RangeID)
 	if err := rsl.SynthesizeRaftState(ctx, readWriter); err != nil {
 		log.Fatalf(ctx, "%v", err)
 	}
+	if err := rsl.SetRaftTruncatedState(ctx, readWriter, &kvserverpb.RaftTruncatedState{
+		Index: stateloader.RaftInitialLogIndex,
+		Term:  stateloader.RaftInitialLogTerm,
+	}); err != nil {
+		log.Fatalf(ctx, "%v", err)
+	}
 	// Persist the closed timestamp.
 	//
 	// In order to tolerate a nil initClosedTS input, let's forward to

pkg/kv/kvserver/store_test.go

@@ -72,6 +72,7 @@ import (
 	"github.com/cockroachdb/redact"
 	"github.com/kr/pretty"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/exp/slices"
 	"golang.org/x/sync/errgroup"
 )
 
@@ -4128,6 +4129,77 @@ func TestStoreGetOrCreateReplicaWritesRaftReplicaID(t *testing.T) {
 	require.Equal(t, kvserverpb.RaftReplicaID{ReplicaID: 7}, replicaID)
 }
 
+// TestSplitPreApplyInitializesTruncatedState ensures that the Raft truncated
+// state for the RHS is correctly initialized when calling splitPreApply.
+func TestSplitPreApplyInitializesTruncatedState(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	manual := timeutil.NewManualTime(timeutil.Unix(0, 10))
+	clock := hlc.NewClockForTesting(manual)
+
+	db := storage.NewDefaultInMemForTesting()
+	defer db.Close()
+	batch := db.NewBatch()
+	defer batch.Close()
+
+	startKey := roachpb.Key("0000")
+	endKey := roachpb.Key("9999")
+	desc := roachpb.RangeDescriptor{
+		RangeID:  99,
+		StartKey: roachpb.RKey(startKey),
+		EndKey:   roachpb.RKey(endKey),
+	}
+	desc.AddReplica(1, 1, roachpb.VOTER_FULL)
+
+	sl := stateloader.Make(desc.RangeID)
+	// Write the range state that will be consulted and copied during the split.
+	lease := roachpb.Lease{
+		Replica:       desc.InternalReplicas[0],
+		Term:          10,
+		MinExpiration: hlc.Timestamp{WallTime: 100},
+	}
+	err := sl.SetLease(ctx, batch, nil, lease)
+	require.NoError(t, err)
+
+	// Set up the store and LHS replica.
+	cfg := TestStoreConfig(clock)
+	stopper := stop.NewStopper()
+	defer stopper.Stop(ctx)
+	store := createTestStoreWithConfig(ctx, t, stopper, testStoreOpts{}, &cfg)
+	lhsRepl := createReplica(store, desc.RangeID, desc.StartKey, desc.EndKey)
+
+	// Construct the split trigger.
+	splitKey := roachpb.RKey("5555")
+	leftDesc, rightDesc := desc, desc
+	leftDesc.EndKey = splitKey
+	rightDesc.RangeID++
+	rightDesc.StartKey = splitKey
+	rightDesc.InternalReplicas = slices.Clone(leftDesc.InternalReplicas)
+	rightDesc.InternalReplicas[0].ReplicaID++
+
+	// Create an uninitialized replica for the RHS. splitPreApply expects this.
+	_, _, err = store.getOrCreateReplica(
+		ctx, rightDesc.RangeID, rightDesc.InternalReplicas[0].ReplicaID, &rightDesc.InternalReplicas[0],
+	)
+	require.NoError(t, err)
+
+	split := roachpb.SplitTrigger{
+		LeftDesc:  leftDesc,
+		RightDesc: rightDesc,
+	}
+
+	splitPreApply(ctx, lhsRepl, batch, split, nil)
+
+	// Verify that the RHS truncated state is initialized as expected.
+	rsl := stateloader.Make(rightDesc.RangeID)
+	truncState, err := rsl.LoadRaftTruncatedState(ctx, batch)
+	require.NoError(t, err)
+	require.Equal(t, stateloader.RaftInitialLogIndex, int(truncState.Index))
+	require.Equal(t, stateloader.RaftInitialLogTerm, int(truncState.Term))
+}
+
 func BenchmarkStoreGetReplica(b *testing.B) {
 	ctx := context.Background()
 	stopper := stop.NewStopper()

pkg/sql/catalog/descriptor.go

@@ -31,6 +31,11 @@ import (
 // DescriptorType is a symbol representing the (sub)type of a descriptor.
 type DescriptorType string
 
+var _ redact.SafeValue = DescriptorType("")
+
+// SafeValue implements redact.SafeValue.
+func (DescriptorType) SafeValue() {}
+
 const (
 	// Any represents any descriptor.
 	Any DescriptorType = "any"