Merge #73878 #76252 #76617 #76627 #76667

73878: sql: introduce MVCC-compliant index backfiller r=ajwerner a=stevendanna

Previously, the index backfilling process depended upon non-MVCC
compliant AddSSTable calls which potentially rewrote previously read
historical values.

To support an MVCC-compliant AddSSTable that writes at the _current_
timestamp, this change implements a new backfilling process described
in the following RFC:

https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20211004_incremental_index_backfiller.md

In summary, the new index backfilling process depends on backfilling
the new index when it is in a BACKFILLING state (added in #72281). In
this state it receives no writes or deletes. Writes that occur during
the backfilling process are captured by a "temporary index."  This
temporary index uses the DeletePreservingEncoding to ensure it
captures deletes as well as writes.

After the of bulk backfill using the MVCC-compliant AddSSTable, the
index is moved into a MERGING state
(added in #75663) in which it receives writes and deletes. Writes
previously captured by the temporary index are then transactionally
merged into the newly added index.

This feature is currently behind a new boolean cluster setting which
default to true. Schema changes that contains both old and new-style
backfills are rejected.

Reverting the default to false will require updating various tests
since many tests depend on the exact index IDs of newly added indexes.

Release note: None

76252: kvserver: account for AddSSTableRequests in QPS r=kvoli a=kvoli

Previously, Queries-Per-Second (QPS) was calculated uniformly per
`BatchRequest` as 1. This patch introduces variable QPS calculation
for `AddSSTableRequest`, which use an order of magnitude more
resources than other request types.

This patch introduces the
`kv.replica_stats.addsst_request_size_factor` cluster setting. This
setting is used to attribute QPS to `AddSSTableRequest` sizes. The
calculation is done as QPS = 1 + size(AddSSTableRequest) / factor.
When `kv.replica_stats.addsst_request_size_factor` is less than 1, or
no `AddSSTableRequest` exists within a `BatchRequest`, then QPS = 1;
the current behavior today.

resolves #73731

Release note (performance improvement):
Introduced `kv.replica_stats.addsst_request_size_factor` cluster
setting. This setting is used to tune Queries-Per-Second (QPS)
sensitivity to large imports. By default, this setting is disabled.
When enabled, the size of any AddSSTableRequest will contribute to
QPS in inverse relation to this settings magnitude. By default this
setting configured to a conservative 50,000; every 50 kilobytes will
be accounted for as an additional 1 QPS.

76617: bazel: make `CrdbTestBuild` `const` r=RaduBerinde a=rickystewart

This partially reverts #72838. That change was made to avoid thrashing
the cache when swapping between test- and non-test configurations.
Today we have `dev cache` which can retain build artifacts across the
different configurations, so this doesn't really serve a purpose any
more. Indeed, you can now swap between `bazel build
pkg/cmd/cockroach-short` and `bazel build pkg/cmd/cockroach-short
--config=test` very quickly with Bazel downloading old versions of the
built libraries if the built-in cache gets wiped.

We still filter out the `go:build` lines in `crdb_test_{off,on}.go` so
we don't have to set `gotags` for test and non-test, which still saves a
lot of time and unnecessary recompilation. We have a check for this in
CI so no one should be able to add a build constraint without us
knowing.

Release note: None

76627: util/tracing: improve span recording interface r=andreimatei a=andreimatei

Before this patch, the Span's recording interface was left over from a
time when there were only one recording mode: verbose. We now have two
modes: verbose recording and structured recording. They can be enabled
at span creation time through the WithRecording(<type>) option. This
patch changes the Span's SetVerbose() method to expose the two options.

Release note: None

76667: roachtest/test: fix ORM testing due to removed cluster setting r=rafiss a=otan

`sql.catalog.unsafe_skip_system_config_trigger.enabled` got removed
recently and was part of an alpha. Let's clean it up in ORMs too.

Resolves #76654
Resolves #76655
Resolves #76656
Resolves #76657
Resolves #76658
Resolves #76659
Resolves #76660
Resolves #76661
Resolves #76662
Resolves #76663
Resolves #76664
Resolves #76665
Resolves #76666

Release note: None

Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: Rui Hu <[email protected]>
Co-authored-by: Austen McClernon <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>

BUILD.bazel

@@ -123,7 +123,6 @@ exports_files([
 # gazelle:exclude pkg/**/*_string.go
 # gazelle:exclude pkg/ui/distccl/distccl_no_bazel.go
 # gazelle:exclude pkg/ui/distoss/distoss_no_bazel.go
-# gazelle:exclude pkg/util/buildutil/crdb_test_dyn.go
 # gazelle:exclude pkg/util/buildutil/crdb_test_off.go
 # gazelle:exclude pkg/util/buildutil/crdb_test_on.go
 # gazelle:exclude pkg/acceptance/compose/gss/psql/*

build/bazelutil/check.sh

@@ -54,7 +54,6 @@ pkg/cmd/teamcity-trigger/BUILD.bazel
 "
 
 EXISTING_CRDB_TEST_BUILD_CONSTRAINTS="
-pkg/util/buildutil/crdb_test_dyn.go://go:build bazel
 pkg/util/buildutil/crdb_test_off.go://go:build !crdb_test || crdb_test_off
 pkg/util/buildutil/crdb_test_on.go://go:build crdb_test && !crdb_test_off
 "

build/toolchains/BUILD.bazel

@@ -124,23 +124,10 @@ platform(
 )
 
 config_setting(
-    name = "dbg_crdb_test",
+    name = "crdb_test",
     define_values = {
         "crdb_test": "y",
     },
-    values = {
-        "compilation_mode": "dbg",
-    },
-)
-
-config_setting(
-    name = "fastbuild_crdb_test",
-    define_values = {
-        "crdb_test": "y",
-    },
-    values = {
-        "compilation_mode": "fastbuild",
-    },
 )
 
 config_setting(
@@ -177,16 +164,6 @@ config_setting(
     },
 )
 
-config_setting(
-    name = "opt_crdb_test",
-    define_values = {
-        "crdb_test": "y",
-    },
-    values = {
-        "compilation_mode": "opt",
-    },
-)
-
 config_setting(
     name = "opt",
     values = {

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

@@ -33,6 +33,7 @@ kv.protectedts.reconciliation.interval	duration	5m0s	the frequency for reconcili
 kv.range_split.by_load_enabled	boolean	true	allow automatic splits of ranges based on where load is concentrated
 kv.range_split.load_qps_threshold	integer	2500	the QPS over which, the range becomes a candidate for load based splitting
 kv.rangefeed.enabled	boolean	false	if set, rangefeed registration is enabled
+kv.replica_stats.addsst_request_size_factor	integer	50000	the divisor that is applied to addsstable request sizes, then recorded in a leaseholders QPS; 0 means all requests are treated as cost 1
 kv.replication_reports.interval	duration	1m0s	the frequency for generating the replication_constraint_stats, replication_stats_report and replication_critical_localities reports (set to 0 to disable)
 kv.transaction.max_intents_bytes	integer	4194304	maximum number of bytes used to track locks in transactions
 kv.transaction.max_refresh_spans_bytes	integer	256000	maximum number of bytes used to track refresh spans in serializable transactions
@@ -176,4 +177,4 @@ trace.debug.enable	boolean	false	if set, traces for recent requests can be seen
 trace.jaeger.agent	string		the address of a Jaeger agent to receive traces using the Jaeger UDP Thrift protocol, as <host>:<port>. If no port is specified, 6381 will be used.
 trace.opentelemetry.collector	string		address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.
 trace.zipkin.collector	string		the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.
-version	version	21.2-66	set the active cluster version in the format '<major>.<minor>'
+version	version	21.2-68	set the active cluster version in the format '<major>.<minor>'

docs/generated/settings/settings.html

@@ -42,6 +42,7 @@
 <tr><td><code>kv.range_split.load_qps_threshold</code></td><td>integer</td><td><code>2500</code></td><td>the QPS over which, the range becomes a candidate for load based splitting</td></tr>
 <tr><td><code>kv.rangefeed.enabled</code></td><td>boolean</td><td><code>false</code></td><td>if set, rangefeed registration is enabled</td></tr>
 <tr><td><code>kv.replica_circuit_breaker.slow_replication_threshold</code></td><td>duration</td><td><code>0s</code></td><td>duration after which slow proposals trip the per-Replica circuit breaker (zero duration disables breakers)</td></tr>
+<tr><td><code>kv.replica_stats.addsst_request_size_factor</code></td><td>integer</td><td><code>50000</code></td><td>the divisor that is applied to addsstable request sizes, then recorded in a leaseholders QPS; 0 means all requests are treated as cost 1</td></tr>
 <tr><td><code>kv.replication_reports.interval</code></td><td>duration</td><td><code>1m0s</code></td><td>the frequency for generating the replication_constraint_stats, replication_stats_report and replication_critical_localities reports (set to 0 to disable)</td></tr>
 <tr><td><code>kv.snapshot_rebalance.max_rate</code></td><td>byte size</td><td><code>32 MiB</code></td><td>the rate limit (bytes/sec) to use for rebalance and upreplication snapshots</td></tr>
 <tr><td><code>kv.snapshot_recovery.max_rate</code></td><td>byte size</td><td><code>32 MiB</code></td><td>the rate limit (bytes/sec) to use for recovery snapshots</td></tr>
@@ -189,6 +190,6 @@
 <tr><td><code>trace.jaeger.agent</code></td><td>string</td><td><code></code></td><td>the address of a Jaeger agent to receive traces using the Jaeger UDP Thrift protocol, as <host>:<port>. If no port is specified, 6381 will be used.</td></tr>
 <tr><td><code>trace.opentelemetry.collector</code></td><td>string</td><td><code></code></td><td>address of an OpenTelemetry trace collector to receive traces using the otel gRPC protocol, as <host>:<port>. If no port is specified, 4317 will be used.</td></tr>
 <tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>the address of a Zipkin instance to receive traces, as <host>:<port>. If no port is specified, 9411 will be used.</td></tr>
-<tr><td><code>version</code></td><td>version</td><td><code>21.2-66</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
+<tr><td><code>version</code></td><td>version</td><td><code>21.2-68</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>

pkg/ccl/backupccl/restore_job.go

@@ -1902,7 +1902,7 @@ func (r *restoreResumer) publishDescriptors(
 			}
 			newIdx := found.IndexDescDeepCopy()
 			mutTable.RemovePublicNonPrimaryIndex(found.Ordinal())
-			if err := mutTable.AddIndexMutation(&newIdx, descpb.DescriptorMutation_ADD); err != nil {
+			if err := mutTable.AddIndexMutation(ctx, &newIdx, descpb.DescriptorMutation_ADD, r.settings); err != nil {
 				return err
 			}
 		}

pkg/ccl/backupccl/restore_schema_change_creation.go

@@ -168,7 +168,7 @@ func createSchemaChangeJobsFromMutations(
 ) error {
 	mutationJobs := make([]descpb.TableDescriptor_MutationJob, 0, len(tableDesc.Mutations))
 	seenMutations := make(map[descpb.MutationID]bool)
-	for _, mutation := range tableDesc.Mutations {
+	for idx, mutation := range tableDesc.Mutations {
 		if seenMutations[mutation.MutationID] {
 			// We've already seen a mutation with this ID, so a job that handles all
 			// mutations with this ID has already been created.
@@ -182,7 +182,17 @@ func createSchemaChangeJobsFromMutations(
 		}
 		spanList := make([]jobspb.ResumeSpanList, mutationCount)
 		for i := range spanList {
-			spanList[i] = jobspb.ResumeSpanList{ResumeSpans: []roachpb.Span{tableDesc.PrimaryIndexSpan(codec)}}
+			mut := tableDesc.Mutations[idx+i]
+			// Index mutations with UseDeletePreservingEncoding are
+			// used as temporary indexes that are merged back into
+			// newly added indexes. Their resume spans are based on
+			// the index span itself since we iterate over the
+			// temporary index during the merge process.
+			if idx := mut.GetIndex(); idx != nil && idx.UseDeletePreservingEncoding {
+				spanList[i] = jobspb.ResumeSpanList{ResumeSpans: []roachpb.Span{tableDesc.IndexSpan(codec, idx.ID)}}
+			} else {
+				spanList[i] = jobspb.ResumeSpanList{ResumeSpans: []roachpb.Span{tableDesc.PrimaryIndexSpan(codec)}}
+			}
 		}
 		jobRecord := jobs.Record{
 			// We indicate that this schema change was triggered by a RESTORE since

pkg/ccl/multiregionccl/BUILD.bazel

@@ -42,6 +42,7 @@ go_test(
         "//pkg/ccl/testutilsccl",
         "//pkg/ccl/utilccl",
         "//pkg/jobs",
+        "//pkg/jobs/jobspb",
         "//pkg/keys",
         "//pkg/kv",
         "//pkg/kv/kvbase",

pkg/ccl/multiregionccl/regional_by_row_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/testutilsccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
+	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
@@ -568,6 +569,10 @@ func TestIndexCleanupAfterAlterFromRegionalByRow(t *testing.T) {
 		{locality: "REGIONAL BY ROW AS region_col"},
 	} {
 		t.Run(tc.locality, func(t *testing.T) {
+			// Don't allow gc jobs to complete so that we
+			// can validate that they were created.
+			blockGC := make(chan struct{})
+
 			knobs := base.TestingKnobs{
 				Store: &kvserver.StoreTestingKnobs{
 					// Disable the merge queue because it makes this test flakey
@@ -583,6 +588,7 @@ func TestIndexCleanupAfterAlterFromRegionalByRow(t *testing.T) {
 				},
 				// Decrease the adopt loop interval so that retries happen quickly.
 				JobsTestingKnobs: jobs.NewTestingKnobsWithShortIntervals(),
+				GCJob:            &sql.GCJobTestingKnobs{RunBeforeResume: func(_ jobspb.JobID) error { <-blockGC; return nil }},
 			}
 
 			_, sqlDB, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster(
@@ -680,21 +686,32 @@ CREATE TABLE regional_by_row (
 				return nil
 			}
 
+			expectedGCJobsForDrops := 4
+			expectedGCJobsForTempIndexes := 4
 			// Now check that we have the right number of index GC jobs pending.
-			err = queryIndexGCJobsAndValidateCount(`running`, 4)
+			err = queryIndexGCJobsAndValidateCount(`running`, expectedGCJobsForDrops+expectedGCJobsForTempIndexes)
 			require.NoError(t, err)
 			err = queryIndexGCJobsAndValidateCount(`succeeded`, 0)
 			require.NoError(t, err)
 
+			queryAndEnsureThatIndexGCJobsSucceeded := func(count int) func() error {
+				return func() error { return queryIndexGCJobsAndValidateCount(`succeeded`, count) }
+			}
+
+			// Unblock GC jobs.
+			close(blockGC)
+			// The GC jobs for the temporary indexes should be cleaned up immediately.
+			testutils.SucceedsSoon(t, queryAndEnsureThatIndexGCJobsSucceeded(expectedGCJobsForTempIndexes))
+			// The GC jobs for the drops should still be waiting out the GC TTL.
+			err = queryIndexGCJobsAndValidateCount(`running`, expectedGCJobsForDrops)
+			require.NoError(t, err)
+
 			// Change gc.ttlseconds to speed up the cleanup.
 			_, err = sqlDB.Exec(`ALTER TABLE regional_by_row CONFIGURE ZONE USING gc.ttlseconds = 1`)
 			require.NoError(t, err)
 
 			// Validate that indexes are cleaned up.
-			queryAndEnsureThatFourIndexGCJobsSucceeded := func() error {
-				return queryIndexGCJobsAndValidateCount(`succeeded`, 4)
-			}
-			testutils.SucceedsSoon(t, queryAndEnsureThatFourIndexGCJobsSucceeded)
+			testutils.SucceedsSoon(t, queryAndEnsureThatIndexGCJobsSucceeded(expectedGCJobsForDrops+expectedGCJobsForTempIndexes))
 			err = queryIndexGCJobsAndValidateCount(`running`, 0)
 			require.NoError(t, err)
 		})
@@ -918,7 +935,7 @@ func TestIndexDescriptorUpdateForImplicitColumns(t *testing.T) {
 
 	t.Run("primary index", func(t *testing.T) {
 		tdb.Exec(t, `CREATE TABLE test.t1 (
-			a INT PRIMARY KEY, 
+			a INT PRIMARY KEY,
 			b test.public.crdb_internal_region NOT NULL
 		) LOCALITY GLOBAL`)
 		indexes := fetchIndexes("t1")
@@ -944,7 +961,7 @@ func TestIndexDescriptorUpdateForImplicitColumns(t *testing.T) {
 
 	t.Run("secondary index", func(t *testing.T) {
 		tdb.Exec(t, `CREATE TABLE test.t2 (
-			a INT PRIMARY KEY, 
+			a INT PRIMARY KEY,
 			b test.public.crdb_internal_region NOT NULL,
 			c INT NOT NULL,
 			d INT NOT NULL,

pkg/ccl/partitionccl/drop_test.go

@@ -104,8 +104,8 @@ func TestDropIndexWithZoneConfigCCL(t *testing.T) {
 		partition string
 	}{
 		{1, ""},
-		{3, ""},
-		{3, "p2"},
+		{4, ""},
+		{4, "p2"},
 	}
 	for _, target := range subzones {
 		if exists := subzoneExists(cfg, target.index, target.partition); !exists {
@@ -157,7 +157,7 @@ func TestDropIndexPartitionedByUserDefinedTypeCCL(t *testing.T) {
 		t.Helper()
 		var id int
 		tdb.QueryRow(t, `
-SELECT job_id 
+SELECT job_id
   FROM crdb_internal.jobs
  WHERE description LIKE $1
 `, description).Scan(&id)

pkg/ccl/partitionccl/partition_test.go

@@ -1462,12 +1462,12 @@ ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (y)
 
 	// Our subzones should be spans prefixed with dropped copy of i1,
 	// dropped copy of i2, new copy of i1, and new copy of i2.
-	// These have ID's 2, 3, 6 and 7 respectively.
+	// These have ID's 2, 3, 8 and 10 respectively.
 	expectedSpans := []roachpb.Key{
 		table.IndexSpan(keys.SystemSQLCodec, 2 /* indexID */).Key,
 		table.IndexSpan(keys.SystemSQLCodec, 3 /* indexID */).Key,
-		table.IndexSpan(keys.SystemSQLCodec, 6 /* indexID */).Key,
-		table.IndexSpan(keys.SystemSQLCodec, 7 /* indexID */).Key,
+		table.IndexSpan(keys.SystemSQLCodec, 8 /* indexID */).Key,
+		table.IndexSpan(keys.SystemSQLCodec, 10 /* indexID */).Key,
 	}
 	if len(zone.SubzoneSpans) != len(expectedSpans) {
 		t.Fatalf("expected subzones to have length %d", len(expectedSpans))

pkg/clusterversion/cockroach_versions.go

@@ -283,6 +283,12 @@ const (
 	// to disable the data propagation mechanism it and the entire spanconfig
 	// infrastructure obviates.
 	DisableSystemConfigGossipTrigger
+	// MVCCIndexBackfiller supports MVCC-compliant index
+	// backfillers via a new BACKFILLING index state, delete
+	// preserving temporary indexes, and a post-backfill merging
+	// processing.
+	MVCCIndexBackfiller
+
 	// *************************************************
 	// Step (1): Add new versions here.
 	// Do not add new versions to a patch release.
@@ -450,7 +456,10 @@ var versionsSingleton = keyedVersions{
 		Key:     DisableSystemConfigGossipTrigger,
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 66},
 	},
-
+	{
+		Key:     MVCCIndexBackfiller,
+		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 68},
+	},
 	// *************************************************
 	// Step (2): Add new versions here.
 	// Do not add new versions to a patch release.

pkg/cmd/roachtest/tests/orm_helpers.go

@@ -48,7 +48,6 @@ func alterZoneConfigAndClusterSettings(
 		`SET CLUSTER SETTING jobs.retention_time = '15s';`,
 		`SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false;`,
 		`SET CLUSTER SETTING kv.range_split.by_load_merge_delay = '5s';`,
-		`SET CLUSTER SETTING sql.catalog.unsafe_skip_system_config_trigger.enabled = true;`,
 
 		// Enable experimental features.
 		`SET CLUSTER SETTING sql.defaults.experimental_temporary_tables.enabled = 'true';`,

pkg/jobs/errors.go

@@ -48,6 +48,12 @@ func IsPermanentJobError(err error) bool {
 	return errors.Is(err, errJobPermanentSentinel)
 }
 
+// IsPauseSelfError checks whether the given error is a
+// PauseRequestError.
+func IsPauseSelfError(err error) bool {
+	return errors.Is(err, errPauseSelfSentinel)
+}
+
 // errPauseSelfSentinel exists so the errors returned from PauseRequestErr can
 // be marked with it.
 var errPauseSelfSentinel = errors.New("job requested it be paused")

pkg/kv/kvserver/BUILD.bazel

@@ -377,6 +377,7 @@ go_test(
         "//pkg/testutils/serverutils",
         "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
+        "//pkg/testutils/sstutil",
         "//pkg/testutils/testcluster",
         "//pkg/ts",
         "//pkg/ts/tspb",

pkg/kv/kvserver/allocator_test.go

@@ -5110,17 +5110,17 @@ func TestAllocatorTransferLeaseTargetLoadBased(t *testing.T) {
 	// the unknown node 99 in evenlyBalanced to verify that requests from
 	// unknown localities don't affect the algorithm.
 	evenlyBalanced := newReplicaStats(clock, localityFn)
-	evenlyBalanced.record(1)
-	evenlyBalanced.record(2)
-	evenlyBalanced.record(3)
+	evenlyBalanced.recordCount(1, 1)
+	evenlyBalanced.recordCount(1, 2)
+	evenlyBalanced.recordCount(1, 3)
 	imbalanced1 := newReplicaStats(clock, localityFn)
 	imbalanced2 := newReplicaStats(clock, localityFn)
 	imbalanced3 := newReplicaStats(clock, localityFn)
 	for i := 0; i < 100*int(MinLeaseTransferStatsDuration.Seconds()); i++ {
-		evenlyBalanced.record(99)
-		imbalanced1.record(1)
-		imbalanced2.record(2)
-		imbalanced3.record(3)
+		evenlyBalanced.recordCount(1, 99)
+		imbalanced1.recordCount(1, 1)
+		imbalanced2.recordCount(1, 2)
+		imbalanced3.recordCount(1, 3)
 	}
 
 	manual.Increment(int64(MinLeaseTransferStatsDuration))

pkg/kv/kvserver/batcheval/cmd_add_sstable_test.go

@@ -623,16 +623,16 @@ func TestEvalAddSSTable(t *testing.T) {
 			expectStatsEst: true,
 		},
 		/* Disabled due to nondeterminism under metamorphic tests. SSTTimestamp will
-		 * shortly be removed anyway.
-		"SSTTimestamp doesn't rewrite with incorrect timestamp, but errors under race": {
-			atReqTS:        8,
-			data:           []sstutil.KV{{"a", 6, "a6"}},
-			sst:            []sstutil.KV{{"a", 7, "a7"}},
-			sstTimestamp:   8,
-			expect:         []sstutil.KV{{"a", 7, "a7"}, {"a", 6, "a6"}},
-			expectErrRace:  `incorrect timestamp 0.000000007,0 for SST key "a" (expected 0.000000008,0)`,
-			expectStatsEst: true,
-		},*/
+		    * shortly be removed anyway.
+		   "SSTTimestamp doesn't rewrite with incorrect timestamp, but errors under race": {
+		   	atReqTS:        8,
+		   	data:           []sstutil.KV{{"a", 6, "a6"}},
+		   	sst:            []sstutil.KV{{"a", 7, "a7"}},
+		   	sstTimestamp:   8,
+		   	expect:         []sstutil.KV{{"a", 7, "a7"}, {"a", 6, "a6"}},
+		   	expectErrRace:  `incorrect timestamp 0.000000007,0 for SST key "a" (expected 0.000000008,0)`,
+		   	expectStatsEst: true,
+		   },*/
 	}
 	testutils.RunTrueAndFalse(t, "IngestAsWrites", func(t *testing.T, ingestAsWrites bool) {
 		for name, tc := range testcases {

pkg/kv/kvserver/replica_metrics.go

@@ -249,8 +249,8 @@ func calcBehindCount(
 // A "Query" is a BatchRequest (regardless of its contents) arriving at the
 // leaseholder with a gateway node set in the header (i.e. excluding requests
 // that weren't sent through a DistSender, which in practice should be
-// practically none). Also return the amount of time over which the stat was
-// accumulated.
+// practically none). See Replica.getBatchRequestQPS() for how this is
+// accounted for.
 func (r *Replica) QueriesPerSecond() (float64, time.Duration) {
 	return r.leaseholderStats.avgQPS()
 }