kvserver/batcheval: ignore UseRangeTombstonesForPointDeletes knob som…

…etimes

We want to ignore the point deletion knob for cases where we're deleting data
from system tables watched by rangefeeds. Not doing so can lead to missed
updates. This is particularly critical for cluster settings.

Fixes cockroachdb#87575
Fixes cockroachdb#87479

Release note: None

pkg/storage/mvcc.go

@@ -6269,8 +6269,9 @@ func ReplacePointTombstonesWithRangeTombstones(
 
 		key := iter.UnsafeKey()
 
-		// Skip intents and inline values.
-		if key.Timestamp.IsEmpty() {
+		// Skip intents and inline values, and system table keys which
+		// might be watched by rangefeeds.
+		if key.Timestamp.IsEmpty() || isWatchedSystemTable(key.Key) {
 			iter.NextKey()
 			continue
 		}
@@ -6362,3 +6363,29 @@ func ReplacePointTombstonesWithRangeTombstones(
 
 	return nil
 }
+
+// In order to test the correctness of range deletion tombstones, we added a
+// testing knob to replace point deletions with range deletion tombstones in
+// some tests. Unfortuantely, doing so affects the correctness of rangefeeds.
+// The tests in question do not use rangefeeds, but some system functionality
+// does use rangefeeds internally. The primary impact is that catch-up scans
+// will miss deletes. That makes these issues rare and hard to detect. In order
+// to deflake these tests, we avoid rewriting deletes on relevant system
+// tables.
+func isWatchedSystemTable(key roachpb.Key) bool {
+	rem, _, err := keys.DecodeTenantPrefix(key)
+	if err != nil { // allow unprefixed keys to pass through
+		return false
+	}
+	_, tableID, _, err := keys.DecodeTableIDIndexID(rem)
+	if err != nil { // allow keys which do not correspond to sql tables
+		return false
+	}
+	switch tableID {
+	case keys.SettingsTableID, keys.SpanConfigurationsTableID,
+		keys.SQLInstancesTableID, keys.DescriptorTableID, keys.ZonesTableID:
+		return true
+	default:
+		return false
+	}
+}