From 0e8a11a5811155ca506838c550730b20258f82dc Mon Sep 17 00:00:00 2001
From: Radu Berinde <radu@cockroachlabs.com>
Date: Thu, 15 Aug 2024 11:40:30 -0700
Subject: [PATCH] metamorphic: no synthetic suffix with overlapping
 RangeKeySets

Applying a synthetic suffix to overlapping `RangeKeySet`s leads to a
logical ambiguity - they both have the same suffix but they can have
different values. In practical terms it causes metamorphic test
failures because their relative sorting order is indeterminate.

Add code in the metamorphic tests to keep track of `RangeKeySet`s and
prevent the use of synthetic suffix when the external object has
overlapping `RangeKeySet`s.

Fixes #3833
Fixes #3824
---
 metamorphic/generator.go   |  7 +++++++
 metamorphic/key_manager.go | 35 ++++++++++++++++++++++++++++++++---
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/metamorphic/generator.go b/metamorphic/generator.go
index 99e4bd719c..a582935622 100644
--- a/metamorphic/generator.go
+++ b/metamorphic/generator.go
@@ -1415,6 +1415,13 @@ func (g *generator) writerIngestExternalFiles() {
 				continue
 			}
 
+			// We can only use a synthetic suffix if we don't have overlapping range
+			// key sets (because they will become logically conflicting when we
+			// replace their suffixes with the synthetic one).
+			if g.keyManager.ExternalObjectHasOverlappingRangeKeySets(objs[i].externalObjID) {
+				continue
+			}
+
 			// Generate a suffix that sorts before any previously generated suffix.
 			objs[i].syntheticSuffix = g.keyGenerator.IncMaxSuffix()
 		}
diff --git a/metamorphic/key_manager.go b/metamorphic/key_manager.go
index ed713b90f9..230c32cb44 100644
--- a/metamorphic/key_manager.go
+++ b/metamorphic/key_manager.go
@@ -218,6 +218,9 @@ type objKeyMeta struct {
 	hasRangeDels     bool
 	hasRangeKeys     bool
 	hasRangeKeyUnset bool
+	// List of RangeKeySets for this object. Used to check for overlapping
+	// RangeKeySets in external files.
+	rangeKeySets []pebble.KeyRange
 }
 
 // MergeKey adds the given key at the given meta timestamp, merging the histories as needed.
@@ -252,6 +255,7 @@ func (okm *objKeyMeta) MergeFrom(from *objKeyMeta, metaTimestamp int, cmp base.C
 	okm.hasRangeDels = okm.hasRangeDels || from.hasRangeDels
 	okm.hasRangeKeys = okm.hasRangeKeys || from.hasRangeKeys
 	okm.hasRangeKeyUnset = okm.hasRangeKeyUnset || from.hasRangeKeyUnset
+	okm.rangeKeySets = append(okm.rangeKeySets, from.rangeKeySets...)
 }
 
 // objKeyMeta looks up the objKeyMeta for a given object, creating it if necessary.
@@ -325,6 +329,25 @@ func (k *keyManager) KeysForExternalIngest(obj externalObjWithBounds) []keyMeta
 	return res
 }
 
+func (k *keyManager) ExternalObjectHasOverlappingRangeKeySets(externalObjID objID) bool {
+	meta := k.objKeyMeta(externalObjID)
+	if len(meta.rangeKeySets) == 0 {
+		return false
+	}
+	ranges := meta.rangeKeySets
+	// Sort by start key.
+	slices.SortFunc(ranges, func(a, b pebble.KeyRange) int {
+		return k.comparer.Compare(a.Start, b.Start)
+	})
+	// Check overlap between adjacent ranges.
+	for i := 0; i < len(ranges)-1; i++ {
+		if ranges[i].OverlapsKeyRange(k.comparer.Compare, ranges[i+1]) {
+			return true
+		}
+	}
+	return false
+}
+
 func (k *keyManager) nextMetaTimestamp() int {
 	ret := k.metaTimestamp
 	k.metaTimestamp++
@@ -614,11 +637,17 @@ func (k *keyManager) update(o op) {
 		k.objKeyMeta(s.writerID).hasRangeKeys = true
 	case *rangeKeySetOp:
 		k.expandBounds(s.writerID, k.makeEndExclusiveBounds(s.start, s.end))
-		k.objKeyMeta(s.writerID).hasRangeKeys = true
+		meta := k.objKeyMeta(s.writerID)
+		meta.hasRangeKeys = true
+		meta.rangeKeySets = append(meta.rangeKeySets, pebble.KeyRange{
+			Start: s.start,
+			End:   s.end,
+		})
 	case *rangeKeyUnsetOp:
 		k.expandBounds(s.writerID, k.makeEndExclusiveBounds(s.start, s.end))
-		k.objKeyMeta(s.writerID).hasRangeKeys = true
-		k.objKeyMeta(s.writerID).hasRangeKeyUnset = true
+		meta := k.objKeyMeta(s.writerID)
+		meta.hasRangeKeys = true
+		meta.hasRangeKeyUnset = true
 	case *ingestOp:
 		// Some ingestion operations may attempt to ingest overlapping sstables
 		// which is prohibited. We know at generation time whether these