kv: replica inconsistency false positive in mixed-version cluster with global table

[nvanbenschoten]

In #105523 (v23.2), we removed the logic to decode the synthetic timestamp bit from mvcc keys. Keys with synthetic timestamps set would be decoded as if their synthetic timestamp bit was not set. Under normal operation, this was safe, as this field is no longer consulted and is being phased out (#101938).

The idea was that this would not cause issues with mixed-version clusters (v23.1 + v23.2) because replica consistency checks operated at the encoded EngineKey level, so previously present synthetic timestamps would be included in replica consistency checksum even from v23.2 nodes.

However, in the process of completing the migration and adding more mixed-version testing (#117294), I am finding that this assumption is false. Replica consistency checks operate at the MVCCKey level, meaning that they decode each encoded pebble key into an MVCC key and then hash the timestamp component separately:

cockroach/pkg/kv/kvserver/replica_consistency.go

Lines 559 to 565 in 797e658

	legacyTimestamp = unsafeKey.Timestamp.ToLegacyTimestamp()
	if size := legacyTimestamp.Size(); size > cap(timestampBuf) {
	timestampBuf = make([]byte, size)
	} else {
	timestampBuf = timestampBuf[:size]
	}
	if _, err := protoutil.MarshalToSizedBuffer(&legacyTimestamp, timestampBuf); err != nil {

This is a problem. If a v23.2 node does not decode the synthetic timestamp bit and a v23.1 node does, then it will hash the timestamp component differently than a v23.1 node will, and the replica consistency check will fail. I can reproduce this with the following script:

roachprod create local -n3
roachprod stage local:1,2 release v23.1.11
roachprod stage local:3 release v23.2.0-rc.1
roachprod start local

roachprod sql local:1 -- -e "SET CLUSTER SETTING server.consistency_check.interval = '0'"
roachprod run local:1 -- cockroach workload init kv --splits=9
roachprod sql local:1 -- -e "ALTER TABLE kv.kv SCATTER"

# consistency check succeeds before table marked as global.
roachprod sql local:1 -- -e "SELECT * FROM crdb_internal.check_consistency(false, '', '') WHERE status NOT IN ('RANGE_CONSISTENT', 'RANGE_CONSISTENT_STATS_ESTIMATED')"

# consistency check fails after table marked as global and data written to table.
roachprod sql local:1 -- -e "ALTER TABLE kv.kv CONFIGURE ZONE USING global_reads = true"
roachprod run local:1 -- cockroach workload run kv --duration=10s
roachprod sql local:1 -- -e "SELECT * FROM crdb_internal.check_consistency(false, '', '') WHERE status NOT IN ('RANGE_CONSISTENT', 'RANGE_CONSISTENT_STATS_ESTIMATED')"

Mitigation and Next Steps

Option 1

One mitigation is to revert most or all of #105523. We need to keep decoding synthetic timestamps in MVCC keys until the consistency checks stop consulting them.

To complete the migration, we'll then need to phase out synthetic timestamps in replica consistency checks more slowly. We'll add a version gate flag that gets plumbed down to CalcReplicaDigest and is used to selectively ignore synthetic timestamps.

Option 2

Another mitigation is to bump the ReplicaChecksumVersion in v23.2, so that v23.1 and v23.2 nodes don't mix when performing consistency checks. This would be the safest fix and would also make the migration easy, but it would disable replica consistency checks between v23.1 and v23.2 nodes when in a mixed-version, which might be too much of a loss of coverage to justify.

Jira issue: CRDB-35111

[nvanbenschoten]

Removing the GA-blocker label, now that #117324 and #117341 have been merged.

This will still be an issue on master though, so we still need to land something like #117304 to resolve it for v24.1.