storage: CheckSSTConflicts computes incorrect stats for SST range keys

[erikgrinaker]

In #94140, I'm adding kvnemesis test coverage of AddSSTable ingestion of MVCC range keys. This found a number of problems:

• Conflicting intents are not handled correctly, errors with checking for key collisions: not at MVCC value. Fixed in storage: handle intents under ingested range key in CheckSSTConflicts #94395

• The SST and engine iterators get out of sync, errors with checking for key collisions: expected engine iter to be ahead of sst iter. Fixed in storage: handle intents under ingested range key in CheckSSTConflicts #94395

• Incorrect MVCC stats calculations. Not fixed yet

To reproduce, check out the branch in #94140 and run e.g.:

$ dev test pkg/kv/kvnemesis -f TestKVNemesisSingleNode -v --stress --ignore-cache
=== RUN   TestKVNemesisSingleNode
I221222 15:43:31.221541 1 (gostd) rand.go:199  [-] 1  random seed: -6583167934786910228
    test_log_scope.go:161: test logs captured to: /tmp/test/_tmp/1f42cf5be2fc021646bf9b2daf5eaef3/logTestKVNemesisSingleNode1895708328
    test_log_scope.go:79: use -show-logs to present logs inline
    kvnemesis_test.go:179: seed: 7378274295210697597
    kvnemesis_test.go:123: kvnemesis logging to /tmp/test/_tmp/1f42cf5be2fc021646bf9b2daf5eaef3/kvnemesis2616062505
    kvnemesis.go:165: error applying x.AddSSTable(ctx, tk(3909762554118877548), tk(18446744073709551615), ... /* @s7 */) // 1221 bytes (as writes)
        // ^-- [tk(7945596544032338278), tk(12435402613537045058)) -> sv(s7): /Table/100/"{6e446df58f9e7166"-ac936cca9ad6fa42"} -> /<empty>
        // ^-- [tk(17786095308637885568), tk(18446744073709551615)) -> sv(s7): /Table/100/"f{6d4e772cc9a5480"-fffffffffffffff"} -> /<empty>: checking for key collisions: expected engine iter to be ahead of sst iter
    kvnemesis.go:185: failures(verbose): /tmp/test/_tmp/1f42cf5be2fc021646bf9b2daf5eaef3/kvnemesis2616062505/failures
        repro steps: /tmp/test/_tmp/1f42cf5be2fc021646bf9b2daf5eaef3/kvnemesis2616062505/repro.go
        rangefeed KVs: /tmp/test/_tmp/1f42cf5be2fc021646bf9b2daf5eaef3/kvnemesis2616062505/kvs-rangefeed.txt
        scan KVs: /tmp/test/_tmp/1f42cf5be2fc021646bf9b2daf5eaef3/kvnemesis2616062505/kvs-scan.txt
    kvnemesis_test.go:206: 
        	Error Trace:	/home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/sandbox/linux-sandbox/291/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/kv/kvnemesis/kvnemesis_test_/kvnemesis_test.runfiles/com_github_cockroachdb_cockroach/pkg/kv/kvnemesis/kvnemesis_test.go:206
        	            				/home/erik/.cache/bazel/_bazel_erik/58b8816bb117734f0a0eaaef7fcef95b/sandbox/linux-sandbox/291/execroot/com_github_cockroachdb_cockroach/bazel-out/k8-fastbuild/bin/pkg/kv/kvnemesis/kvnemesis_test_/kvnemesis_test.runfiles/com_github_cockroachdb_cockroach/pkg/kv/kvnemesis/kvnemesis_test.go:147
        	Error:      	Should be zero, but was 1
        	Test:       	TestKVNemesisSingleNode
        	Messages:   	kvnemesis detected failures
    panic.go:522: -- test log scope end --
test logs left over in: /tmp/test/_tmp/1f42cf5be2fc021646bf9b2daf5eaef3/logTestKVNemesisSingleNode1895708328
--- FAIL: TestKVNemesisSingleNode (1.58s)
FAIL
I221222 15:43:32.816153 1 (gostd) testmain.go:100  [-] 1  Test //pkg/kv/kvnemesis:kvnemesis_test exited with error code 1


ERROR: exit status 1

11 runs completed, 1 failures, over 2s

Jira issue: CRDB-22707

Epic CRDB-20465

[blathers-crl]

Hi @erikgrinaker, please add branch-* labels to identify which branch(es) this release-blocker affects.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[itsbilal]

Note that the first two parts of this issue are fixed by #94395. This issue is now solely about the stats divergence picked up by kvnemesis.

[erikgrinaker]

Reopening for tracking, see #98473 and #94876.

I disabled range key ingestion in #98475, make sure to re-enable it when resolving this.

[jbowens]

@erikgrinaker, @itsbilal — given we're playing a whack-a-mole with CheckSSTConflicts bugs here, how do you feel about the risk to 23.1? An unexpected MVCCStats divergence is not treated as a fatal error by the consistency checker, correct? Is there any reason to conservatively mark stats as estimate in error-prone conditions (eg, like the SST contains range keys).

[erikgrinaker]

An unexpected MVCCStats divergence is not treated as a fatal error by the consistency checker, correct?

Correct, they'll simply get corrected by the consistency checker on the next run, with a log message saying as much. We have some heuristics here and there which do rely on MVCC stats, but I don't believe any of this is critical -- for example, the MVCC GC queue will check MVCC stats to determine whether ranges should be aggressively GCed (when a range is fully deleted by an MVCC range tombstone).

cockroach/pkg/kv/kvserver/mvcc_gc_queue.go

Lines 530 to 539 in 736a67e

	// suspectedFullRangeDeletion checks for ranges where there's no live data and
	// range tombstones are present. This is an indication that range is likely
	// removed by bulk operations and its garbage collection should be done faster
	// than if it has to wait for double ttl.
	func suspectedFullRangeDeletion(ms enginepb.MVCCStats) bool {
	if ms.LiveCount > 0 || ms.IntentCount > 0 {
	return false
	}
	return ms.RangeKeyCount > 0
	}

I'm not particularly worried about an occasional MVCC stats error in the wild, and I believe 23.1 will only ingest SSTs with MVCC range tombstones during C2C replication where CheckSSTConflicts will be too expensive anyway (so we'll have to recompute MVCC stats later). Some other bulk operations also already disable CheckSSTConflicts for performance reasons, which can result in incorrect stats (I forget which). We do try to keep MVCC stats accurate when we can though.