Merge #142999

142999: kvserver: actually test lock replication on merge r=yuzefovich a=stevendanna

We modified this test during review and I made a mistake that meant that it always passed because they key being locked wasn't actually in the range being merged.

Here, I fixed that and along they way also make sure that we run the consistency checker on the range after merging.

Epic: none
Release note: None

Co-authored-by: Steven Danna <[email protected]>

Author: craig[bot]

pkg/kv/kvnemesis/BUILD.bazel

@@ -30,6 +30,7 @@ go_library(
         "//pkg/kv/kvserver/concurrency/isolation",
         "//pkg/kv/kvserver/concurrency/lock",
         "//pkg/kv/kvserver/liveness",
+        "//pkg/kv/kvtestutils",
         "//pkg/roachpb",
         "//pkg/settings/cluster",
         "//pkg/sql/catalog/bootstrap",

pkg/kv/kvnemesis/env.go

@@ -9,12 +9,11 @@ import (
 	"context"
 	gosql "database/sql"
 	"fmt"
-	"regexp"
 	"time"
 
 	"github.com/cockroachdb/cockroach-go/v2/crdb"
 	"github.com/cockroachdb/cockroach/pkg/config/zonepb"
-	kvpb "github.com/cockroachdb/cockroach/pkg/kv/kvpb"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvtestutils"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
@@ -46,53 +45,7 @@ func (e *Env) anyNode() *gosql.DB {
 // as a list of errors. RANGE_CONSISTENT_STATS_ESTIMATED is considered a
 // success, since stats estimates are fine (if unfortunate).
 func (e *Env) CheckConsistency(ctx context.Context, span roachpb.Span) []error {
-	rows, err := e.anyNode().QueryContext(ctx, fmt.Sprintf(`
-		SELECT range_id, start_key_pretty, status, detail
-		FROM crdb_internal.check_consistency(false, b'\x%x', b'\x%x')
-		ORDER BY range_id ASC`,
-		span.Key, span.EndKey,
-	))
-	if err != nil {
-		return []error{err}
-	}
-	defer rows.Close()
-
-	var failures []error
-	for rows.Next() {
-		var rangeID int
-		var key, status, detail string
-		if err := rows.Scan(&rangeID, &key, &status, &detail); err != nil {
-			return []error{err}
-		}
-		// NB: There's a known issue that can result in a 10-byte discrepancy in
-		// SysBytes. See:
-		// https://github.com/cockroachdb/cockroach/issues/93896
-		//
-		// This isn't critical, so we ignore such discrepancies.
-		if status == kvpb.CheckConsistencyResponse_RANGE_CONSISTENT_STATS_INCORRECT.String() {
-			m := regexp.MustCompile(`.*\ndelta \(stats-computed\): \{(.*)\}`).FindStringSubmatch(detail)
-			if len(m) > 1 {
-				delta := m[1]
-				// Strip out LastUpdateNanos and all zero-valued fields.
-				delta = regexp.MustCompile(`LastUpdateNanos:\d+`).ReplaceAllString(delta, "")
-				delta = regexp.MustCompile(`\S+:0\b`).ReplaceAllString(delta, "")
-				if regexp.MustCompile(`^\s*SysBytes:-?10\s*$`).MatchString(delta) {
-					continue
-				}
-			}
-		}
-		switch status {
-		case kvpb.CheckConsistencyResponse_RANGE_INDETERMINATE.String():
-			// Can't do anything, so let it slide.
-		case kvpb.CheckConsistencyResponse_RANGE_CONSISTENT.String():
-			// Good.
-		case kvpb.CheckConsistencyResponse_RANGE_CONSISTENT_STATS_ESTIMATED.String():
-			// Ok.
-		default:
-			failures = append(failures, errors.Errorf("range %d (%s) %s:\n%s", rangeID, key, status, detail))
-		}
-	}
-	return failures
+	return kvtestutils.CheckConsistency(ctx, e.anyNode(), span)
 }
 
 // SetClosedTimestampInterval sets the kv.closed_timestamp.target_duration

pkg/kv/kvserver/client_replica_test.go

@@ -39,6 +39,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/protectedts/ptutil"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/raftutil"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/stateloader"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvtestutils"
 	"github.com/cockroachdb/cockroach/pkg/raft"
 	"github.com/cockroachdb/cockroach/pkg/raft/raftpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -5889,20 +5890,28 @@ func TestMergeReplicatesLocks(t *testing.T) {
 
 	for _, b := range []bool{true, false} {
 		name := "lhs-lock"
-		lockKey := lhsKey
+		lockKeySuffix := lhsKey
 		if b {
 			name = "rhs-lock"
-			lockKey = rhsKey
+			lockKeySuffix = rhsKey
 		}
 		t.Run(name, func(t *testing.T) {
 			tc := testcluster.StartTestCluster(t, 3, base.TestClusterArgs{
 				ServerArgs: base.TestServerArgs{
 					Settings: st,
 				},
 			})
+
+			sql := tc.ServerConn(0)
 			defer tc.Stopper().Stop(ctx)
 			scratch := tc.ScratchRange(t)
-			splitKey := append(scratch[:len(scratch):len(scratch)], splitPoint...)
+			mkKey := func(s string) roachpb.Key {
+				prefix := scratch.Clone()
+				return append(prefix[:len(prefix):len(prefix)], s...)
+			}
+
+			lockKey := mkKey(lockKeySuffix)
+			splitKey := mkKey(splitPoint)
 			tc.SplitRangeOrFatal(t, splitKey)
 			// Write a value for the key because at the moment we don't create locks for
 			// non-existent keys.
@@ -5961,6 +5970,13 @@ func TestMergeReplicatesLocks(t *testing.T) {
 			t.Log("cancelling txn2")
 			txn2Cancel()
 			require.NoError(t, g.Wait())
+			failures := kvtestutils.CheckConsistency(ctx, sql, roachpb.Span{
+				Key:    keys.ScratchRangeMin,
+				EndKey: keys.ScratchRangeMax,
+			})
+			for _, err := range failures {
+				t.Errorf("consistency failure: %s", err.Error())
+			}
 		})
 	}
 }

pkg/kv/kvtestutils/BUILD.bazel

@@ -2,14 +2,22 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
 go_library(
     name = "kvtestutils",
-    testonly = 1,
-    srcs = ["test_utils.go"],
+    # This package can become testonly when kvnemesis tests are moved
+    # into a test package.
+    # testonly = 1,
+    srcs = [
+        "consistency.go",
+        "test_utils.go",
+    ],
     importpath = "github.com/cockroachdb/cockroach/pkg/kv/kvtestutils",
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/kv/kvbase",
-        "//pkg/testutils",
+        "//pkg/kv/kvpb",
+        "//pkg/roachpb",
+        "//pkg/sql/pgwire/pgerror",
         "//pkg/util/tracing",
         "//pkg/util/tracing/tracingpb",
+        "@com_github_cockroachdb_errors//:errors",
     ],
 )

pkg/kv/kvtestutils/consistency.go

@@ -0,0 +1,76 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+//
+
+package kvtestutils
+
+import (
+	"context"
+	gosql "database/sql"
+	"fmt"
+	"regexp"
+
+	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/errors"
+)
+
+type querier interface {
+	QueryContext(ctx context.Context, query string, args ...interface{}) (*gosql.Rows, error)
+}
+
+// CheckConsistency runs a consistency check on all ranges in the given span,
+// primarily to verify that MVCC stats are accurate. Any failures are returned
+// as a list of errors. RANGE_CONSISTENT_STATS_ESTIMATED is considered a
+// success, since stats estimates are fine (if unfortunate).
+func CheckConsistency(ctx context.Context, db querier, span roachpb.Span) []error {
+	rows, err := db.QueryContext(ctx, fmt.Sprintf(`
+		SELECT range_id, start_key_pretty, status, detail
+		FROM crdb_internal.check_consistency(false, b'\x%x', b'\x%x')
+		ORDER BY range_id ASC`,
+		span.Key, span.EndKey,
+	))
+	if err != nil {
+		return []error{err}
+	}
+	defer rows.Close()
+
+	var failures []error
+	for rows.Next() {
+		var rangeID int
+		var key, status, detail string
+		if err := rows.Scan(&rangeID, &key, &status, &detail); err != nil {
+			return []error{err}
+		}
+		// NB: There's a known issue that can result in a 10-byte discrepancy in
+		// SysBytes. See:
+		// https://github.com/cockroachdb/cockroach/issues/93896
+		//
+		// This isn't critical, so we ignore such discrepancies.
+		if status == kvpb.CheckConsistencyResponse_RANGE_CONSISTENT_STATS_INCORRECT.String() {
+			m := regexp.MustCompile(`.*\ndelta \(stats-computed\): \{(.*)\}`).FindStringSubmatch(detail)
+			if len(m) > 1 {
+				delta := m[1]
+				// Strip out LastUpdateNanos and all zero-valued fields.
+				delta = regexp.MustCompile(`LastUpdateNanos:\d+`).ReplaceAllString(delta, "")
+				delta = regexp.MustCompile(`\S+:0\b`).ReplaceAllString(delta, "")
+				if regexp.MustCompile(`^\s*SysBytes:-?10\s*$`).MatchString(delta) {
+					continue
+				}
+			}
+		}
+		switch status {
+		case kvpb.CheckConsistencyResponse_RANGE_INDETERMINATE.String():
+			// Can't do anything, so let it slide.
+		case kvpb.CheckConsistencyResponse_RANGE_CONSISTENT.String():
+			// Good.
+		case kvpb.CheckConsistencyResponse_RANGE_CONSISTENT_STATS_ESTIMATED.String():
+			// Ok.
+		default:
+			failures = append(failures, errors.Errorf("range %d (%s) %s:\n%s", rangeID, key, status, detail))
+		}
+	}
+	return failures
+}

pkg/kv/kvtestutils/test_utils.go

@@ -6,10 +6,11 @@
 package kvtestutils
 
 import (
+	"regexp"
 	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/kv/kvbase"
-	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
 )
@@ -54,6 +55,10 @@ func OnlyFollowerReads(rec tracingpb.Recording) bool {
 // kvserver.Is{Retryable,Illegal}ReplicationChangeError,
 // which avoids string matching.
 func IsExpectedRelocateError(err error) bool {
+	if err == nil {
+		return false
+	}
+
 	allowlist := []string{
 		"descriptor changed",
 		"unable to remove replica .* which is not present",
@@ -74,5 +79,9 @@ func IsExpectedRelocateError(err error) bool {
 		"cannot remove learner while snapshot is in flight", // https://github.com/cockroachdb/cockroach/issues/79887
 	}
 	pattern := "(" + strings.Join(allowlist, "|") + ")"
-	return testutils.IsError(err, pattern)
+	matched, merr := regexp.MatchString(pattern, pgerror.FullError(err))
+	if merr != nil {
+		return false
+	}
+	return matched
 }