db: enforce no split user keys in CheckOrdering

compaction.go

@@ -1277,16 +1277,22 @@ func (c *compaction) newInputIter(
 	newIters tableNewIters, newRangeKeyIter keyspan.TableNewSpanIter, snapshots []uint64,
 ) (_ internalIterator, retErr error) {
 	// Validate the ordering of compaction input files for defense in depth.
+	// TODO(jackson): Some of the CheckOrdering calls may be adapted to pass
+	// ProhibitSplitUserKeys if we thread the active format major version in. Or
+	// if we remove support for earlier FMVs, we can remove the parameter
+	// altogether.
 	if len(c.flushing) == 0 {
 		if c.startLevel.level >= 0 {
 			err := manifest.CheckOrdering(c.cmp, c.formatKey,
-				manifest.Level(c.startLevel.level), c.startLevel.files.Iter())
+				manifest.Level(c.startLevel.level), c.startLevel.files.Iter(),
+				manifest.AllowSplitUserKeys)
 			if err != nil {
 				return nil, err
 			}
 		}
 		err := manifest.CheckOrdering(c.cmp, c.formatKey,
-			manifest.Level(c.outputLevel.level), c.outputLevel.files.Iter())
+			manifest.Level(c.outputLevel.level), c.outputLevel.files.Iter(),
+			manifest.AllowSplitUserKeys)
 		if err != nil {
 			return nil, err
 		}
@@ -1296,7 +1302,9 @@ func (c *compaction) newInputIter(
 			}
 			for _, info := range c.startLevel.l0SublevelInfo {
 				err := manifest.CheckOrdering(c.cmp, c.formatKey,
-					info.sublevel, info.Iter())
+					info.sublevel, info.Iter(),
+					// NB: L0 sublevels have never allowed split user keys.
+					manifest.ProhibitSplitUserKeys)
 				if err != nil {
 					return nil, err
 				}
@@ -1308,7 +1316,8 @@ func (c *compaction) newInputIter(
 			}
 			interLevel := c.extraLevels[0]
 			err := manifest.CheckOrdering(c.cmp, c.formatKey,
-				manifest.Level(interLevel.level), interLevel.files.Iter())
+				manifest.Level(interLevel.level), interLevel.files.Iter(),
+				manifest.AllowSplitUserKeys)
 			if err != nil {
 				return nil, err
 			}

compaction_test.go

@@ -1587,7 +1587,8 @@ func TestManualCompaction(t *testing.T) {
 		{
 			testData:   "testdata/manual_compaction_set_with_del",
 			minVersion: FormatBlockPropertyCollector,
-			maxVersion: FormatPrePebblev1MarkedCompacted,
+			// This test exercises split user keys.
+			maxVersion: FormatSplitUserKeysMarkedCompacted - 1,
 		},
 		{
 			testData:   "testdata/singledel_manual_compaction",
@@ -1608,7 +1609,8 @@ func TestManualCompaction(t *testing.T) {
 		{
 			testData:   "testdata/manual_compaction_file_boundaries",
 			minVersion: FormatBlockPropertyCollector,
-			maxVersion: FormatPrePebblev1MarkedCompacted,
+			// This test exercises split user keys.
+			maxVersion: FormatSplitUserKeysMarkedCompacted - 1,
 		},
 		{
 			testData:   "testdata/manual_compaction_file_boundaries_delsized",

format_major_version.go

@@ -219,6 +219,18 @@ func (v FormatMajorVersion) MinTableFormat() sstable.TableFormat {
 	}
 }
 
+// orderingInvariants returns an enum encoding the set of invariants that must
+// hold within the receiver format major version. Invariants only get stricter
+// as the format major version advances, so it is okay to retrieve the
+// invariants from the current format major version and by the time the
+// invariants are enforced, the format major version has advanced.
+func (v FormatMajorVersion) orderingInvariants() manifest.OrderingInvariants {
+	if v < FormatSplitUserKeysMarkedCompacted {
+		return manifest.AllowSplitUserKeys
+	}
+	return manifest.ProhibitSplitUserKeys
+}
+
 // formatMajorVersionMigrations defines the migrations from one format
 // major version to the next. Each migration is defined as a closure
 // which will be invoked on the database before the new format major

get_iter_test.go

@@ -515,7 +515,7 @@ func TestGetIter(t *testing.T) {
 			files[tt.level] = append(files[tt.level], meta)
 		}
 		v := manifest.NewVersion(cmp, base.DefaultFormatter, 10<<20, files)
-		err := v.CheckOrdering(cmp, base.DefaultFormatter)
+		err := v.CheckOrdering(cmp, base.DefaultFormatter, manifest.AllowSplitUserKeys)
 		if tc.badOrdering && err == nil {
 			t.Errorf("desc=%q: want bad ordering, got nil error", desc)
 			continue

internal/keyspan/level_iter_test.go

@@ -309,7 +309,7 @@ func TestLevelIterEquivalence(t *testing.T) {
 				amap[metas[i].FileNum] = metas[i]
 			}
 			b.Added[6] = amap
-			v, err := b.Apply(nil, base.DefaultComparer.Compare, base.DefaultFormatter, 0, 0, nil)
+			v, err := b.Apply(nil, base.DefaultComparer.Compare, base.DefaultFormatter, 0, 0, nil, manifest.ProhibitSplitUserKeys)
 			require.NoError(t, err)
 			levelIter.Init(
 				SpanIterOptions{}, base.DefaultComparer.Compare, tableNewIters,
@@ -448,7 +448,7 @@ func TestLevelIter(t *testing.T) {
 					amap[metas[i].FileNum] = metas[i]
 				}
 				b.Added[6] = amap
-				v, err := b.Apply(nil, base.DefaultComparer.Compare, base.DefaultFormatter, 0, 0, nil)
+				v, err := b.Apply(nil, base.DefaultComparer.Compare, base.DefaultFormatter, 0, 0, nil, manifest.ProhibitSplitUserKeys)
 				require.NoError(t, err)
 				iter = NewLevelIter(
 					SpanIterOptions{}, base.DefaultComparer.Compare,

internal/manifest/l0_sublevels_test.go

@@ -51,7 +51,7 @@ func readManifest(filename string) (*Version, error) {
 		if err := bve.Accumulate(&ve); err != nil {
 			return nil, err
 		}
-		if v, err = bve.Apply(v, base.DefaultComparer.Compare, base.DefaultFormatter, 10<<20, 32000, nil); err != nil {
+		if v, err = bve.Apply(v, base.DefaultComparer.Compare, base.DefaultFormatter, 10<<20, 32000, nil, ProhibitSplitUserKeys); err != nil {
 			return nil, err
 		}
 	}
@@ -448,7 +448,7 @@ func TestL0Sublevels(t *testing.T) {
 			for sublevel, files := range sublevels.levelFiles {
 				slice := NewLevelSliceSpecificOrder(files)
 				err := CheckOrdering(base.DefaultComparer.Compare, base.DefaultFormatter,
-					L0Sublevel(sublevel), slice.Iter())
+					L0Sublevel(sublevel), slice.Iter(), ProhibitSplitUserKeys)
 				if err != nil {
 					return err.Error()
 				}

internal/manifest/testdata/version_check_ordering

@@ -174,6 +174,16 @@ check-ordering
   000001:[a#1,SET-b#2,SET]
   000002:[b#1,SET-d#4,SET]
 ----
+L1 files 000001 and 000002 have overlapping ranges: [a#1,SET-b#2,SET] vs [b#1,SET-d#4,SET]
+1:
+  000001:[a#1,SET-b#2,SET] seqnums:[0-0] points:[a#1,SET-b#2,SET]
+  000002:[b#1,SET-d#4,SET] seqnums:[0-0] points:[b#1,SET-d#4,SET]
+
+check-ordering allow-split-user-keys
+1:
+  000001:[a#1,SET-b#2,SET]
+  000002:[b#1,SET-d#4,SET]
+----
 OK
 
 check-ordering

internal/manifest/testdata/version_edit_apply

@@ -102,11 +102,11 @@ zombies []
 
 apply
  L2
-  3:[a#1,SET-c#2,SET]
+  3:[b#1,SET-c#2,SET]
   4:[d#3,SET-f#4,SET]
-  5:[h#3,SET-j#4,SET]
+  5:[h#3,SET-h#2,SET]
   2:[n#5,SET-q#3,SET]
-  1:[q#2,SET-t#1,SET]
+  1:[r#2,SET-t#1,SET]
 edit
  delete
   L2
@@ -120,9 +120,9 @@ edit
 ----
 2:
   000006:[a#10,SET-a#7,SET]
-  000003:[a#1,SET-c#2,SET]
+  000003:[b#1,SET-c#2,SET]
   000007:[e#1,SET-g#2,SET]
-  000005:[h#3,SET-j#4,SET]
+  000005:[h#3,SET-h#2,SET]
   000010:[j#3,SET-m#2,SET]
   000002:[n#5,SET-q#3,SET]
 zombies [1 4]

internal/manifest/version.go

@@ -1343,16 +1343,20 @@ func (v *Version) Overlaps(
 // CheckOrdering checks that the files are consistent with respect to
 // increasing file numbers (for level 0 files) and increasing and non-
 // overlapping internal key ranges (for level non-0 files).
-func (v *Version) CheckOrdering(cmp Compare, format base.FormatKey) error {
+func (v *Version) CheckOrdering(
+	cmp Compare, format base.FormatKey, order OrderingInvariants,
+) error {
 	for sublevel := len(v.L0SublevelFiles) - 1; sublevel >= 0; sublevel-- {
 		sublevelIter := v.L0SublevelFiles[sublevel].Iter()
-		if err := CheckOrdering(cmp, format, L0Sublevel(sublevel), sublevelIter); err != nil {
+		// Sublevels have NEVER allowed split user keys, so we can pass
+		// ProhibitSplitUserKeys.
+		if err := CheckOrdering(cmp, format, L0Sublevel(sublevel), sublevelIter, ProhibitSplitUserKeys); err != nil {
 			return base.CorruptionErrorf("%s\n%s", err, v.DebugString(format))
 		}
 	}
 
 	for level, lm := range v.Levels {
-		if err := CheckOrdering(cmp, format, Level(level), lm.Iter()); err != nil {
+		if err := CheckOrdering(cmp, format, Level(level), lm.Iter(), order); err != nil {
 			return base.CorruptionErrorf("%s\n%s", err, v.DebugString(format))
 		}
 	}
@@ -1421,10 +1425,34 @@ func (l *VersionList) Remove(v *Version) {
 	v.list = nil // avoid memory leaks
 }
 
+// OrderingInvariants dictates the file ordering invariants active.
+type OrderingInvariants int8
+
+const (
+	// ProhibitSplitUserKeys indicates that adjacent files within a level cannot
+	// contain the same user key.
+	ProhibitSplitUserKeys OrderingInvariants = iota
+	// AllowSplitUserKeys indicates that adjacent files within a level may
+	// contain the same user key. This is only allowed by historical format
+	// major versions.
+	//
+	// TODO(jackson): Remove.
+	AllowSplitUserKeys
+)
+
 // CheckOrdering checks that the files are consistent with respect to
 // seqnums (for level 0 files -- see detailed comment below) and increasing and non-
 // overlapping internal key ranges (for non-level 0 files).
-func CheckOrdering(cmp Compare, format base.FormatKey, level Level, files LevelIterator) error {
+//
+// The ordering field may be passed AllowSplitUserKeys to allow adjacent files that are both
+// inclusive of the same user key. Pebble no longer creates version edits
+// installing such files, and Pebble databases with sufficiently high format
+// major version should no longer have any such files within their LSM.
+// TODO(jackson): Remove AllowSplitUserKeys when we remove support for the
+// earlier format major versions.
+func CheckOrdering(
+	cmp Compare, format base.FormatKey, level Level, files LevelIterator, ordering OrderingInvariants,
+) error {
 	// The invariants to check for L0 sublevels are the same as the ones to
 	// check for all other levels. However, if L0 is not organized into
 	// sublevels, or if all L0 files are being passed in, we do the legacy L0
@@ -1502,11 +1530,29 @@ func CheckOrdering(cmp Compare, format base.FormatKey, level Level, files LevelI
 						prev.Smallest.Pretty(format), prev.Largest.Pretty(format),
 						f.Smallest.Pretty(format), f.Largest.Pretty(format))
 				}
-				if base.InternalCompare(cmp, prev.Largest, f.Smallest) >= 0 {
-					return base.CorruptionErrorf("%s files %s and %s have overlapping ranges: [%s-%s] vs [%s-%s]",
-						errors.Safe(level), errors.Safe(prev.FileNum), errors.Safe(f.FileNum),
-						prev.Smallest.Pretty(format), prev.Largest.Pretty(format),
-						f.Smallest.Pretty(format), f.Largest.Pretty(format))
+
+				// What's considered "overlapping" is dependent on the format
+				// major version. If ordering=ProhibitSplitUserKeys, then both
+				// files cannot contain keys with the same user keys. If the
+				// bounds have the same user key, the previous file's boundary
+				// must have a Trailer indicating that it's exclusive.
+				switch ordering {
+				case AllowSplitUserKeys:
+					if base.InternalCompare(cmp, prev.Largest, f.Smallest) >= 0 {
+						return base.CorruptionErrorf("%s files %s and %s have overlapping ranges: [%s-%s] vs [%s-%s]",
+							errors.Safe(level), errors.Safe(prev.FileNum), errors.Safe(f.FileNum),
+							prev.Smallest.Pretty(format), prev.Largest.Pretty(format),
+							f.Smallest.Pretty(format), f.Largest.Pretty(format))
+					}
+				case ProhibitSplitUserKeys:
+					if v := cmp(prev.Largest.UserKey, f.Smallest.UserKey); v > 0 || (v == 0 && !prev.Largest.IsExclusiveSentinel()) {
+						return base.CorruptionErrorf("%s files %s and %s have overlapping ranges: [%s-%s] vs [%s-%s]",
+							errors.Safe(level), errors.Safe(prev.FileNum), errors.Safe(f.FileNum),
+							prev.Smallest.Pretty(format), prev.Largest.Pretty(format),
+							f.Smallest.Pretty(format), f.Largest.Pretty(format))
+					}
+				default:
+					panic("unreachable")
 				}
 			}
 		}

internal/manifest/version_edit.go

@@ -855,6 +855,7 @@ func AccumulateIncompleteAndApplySingleVE(
 	backingStateMap map[base.DiskFileNum]*FileBacking,
 	addBackingFunc func(*FileBacking),
 	removeBackingFunc func(base.DiskFileNum),
+	orderingInvariants OrderingInvariants,
 ) (_ *Version, zombies map[base.DiskFileNum]uint64, _ error) {
 	if len(ve.RemovedBackingTables) != 0 {
 		panic("pebble: invalid incomplete version edit")
@@ -866,7 +867,7 @@ func AccumulateIncompleteAndApplySingleVE(
 	}
 	zombies = make(map[base.DiskFileNum]uint64)
 	v, err := b.Apply(
-		curr, cmp, formatKey, flushSplitBytes, readCompactionRate, zombies,
+		curr, cmp, formatKey, flushSplitBytes, readCompactionRate, zombies, orderingInvariants,
 	)
 	if err != nil {
 		return nil, nil, err
@@ -907,6 +908,7 @@ func (b *BulkVersionEdit) Apply(
 	flushSplitBytes int64,
 	readCompactionRate int64,
 	zombies map[base.DiskFileNum]uint64,
+	orderingInvariants OrderingInvariants,
 ) (*Version, error) {
 	addZombie := func(state *FileBacking) {
 		if zombies != nil {
@@ -1090,7 +1092,7 @@ func (b *BulkVersionEdit) Apply(
 			} else if err := v.InitL0Sublevels(cmp, formatKey, flushSplitBytes); err != nil {
 				return nil, errors.Wrap(err, "pebble: internal error")
 			}
-			if err := CheckOrdering(cmp, formatKey, Level(0), v.Levels[level].Iter()); err != nil {
+			if err := CheckOrdering(cmp, formatKey, Level(0), v.Levels[level].Iter(), orderingInvariants); err != nil {
 				return nil, errors.Wrap(err, "pebble: internal error")
 			}
 			continue
@@ -1111,7 +1113,7 @@ func (b *BulkVersionEdit) Apply(
 					end.Prev()
 				}
 			})
-			if err := CheckOrdering(cmp, formatKey, Level(level), check.Iter()); err != nil {
+			if err := CheckOrdering(cmp, formatKey, Level(level), check.Iter(), orderingInvariants); err != nil {
 				return nil, errors.Wrap(err, "pebble: internal error")
 			}
 		}

internal/manifest/version_edit_test.go

@@ -514,7 +514,7 @@ func TestVersionEditApply(t *testing.T) {
 					}
 				}
 				zombies := make(map[base.DiskFileNum]uint64)
-				newv, err := bve.Apply(v, base.DefaultComparer.Compare, base.DefaultFormatter, 10<<20, 32000, zombies)
+				newv, err := bve.Apply(v, base.DefaultComparer.Compare, base.DefaultFormatter, 10<<20, 32000, zombies, ProhibitSplitUserKeys)
 				if err != nil {
 					return err.Error()
 				}

internal/manifest/version_test.go

@@ -290,6 +290,10 @@ func TestCheckOrdering(t *testing.T) {
 		func(t *testing.T, d *datadriven.TestData) string {
 			switch d.Cmd {
 			case "check-ordering":
+				orderingInvariants := ProhibitSplitUserKeys
+				if d.HasArg("allow-split-user-keys") {
+					orderingInvariants = AllowSplitUserKeys
+				}
 				v, err := ParseVersionDebug(cmp, fmtKey, 10<<20, d.Input)
 				if err != nil {
 					return err.Error()
@@ -300,7 +304,7 @@ func TestCheckOrdering(t *testing.T) {
 					m.SmallestSeqNum = m.Smallest.SeqNum()
 					m.LargestSeqNum = m.Largest.SeqNum()
 				})
-				if err = v.CheckOrdering(cmp, base.DefaultFormatter); err != nil {
+				if err = v.CheckOrdering(cmp, base.DefaultFormatter, orderingInvariants); err != nil {
 					return err.Error()
 				}
 				return "OK"

replay/replay.go

@@ -718,7 +718,8 @@ func (r *Runner) prepareWorkloadSteps(ctx context.Context) error {
 			r.Opts.Comparer.FormatKey,
 			r.Opts.FlushSplitBytes,
 			r.Opts.Experimental.ReadCompactionRate,
-			nil /* zombies */)
+			nil, /* zombies */
+			manifest.ProhibitSplitUserKeys)
 		bve = manifest.BulkVersionEdit{AddedByFileNum: bve.AddedByFileNum}
 		return v, err
 	}

testdata/compaction_check_ordering

@@ -117,7 +117,7 @@ L0.0
   a.SET.1-b.SET.2
   b.SET.1-d.SET.5
 ----
-OK
+L0.0 files 000001 and 000002 have overlapping ranges: [a#1,SET-b#2,SET] vs [b#1,SET-d#5,SET]
 
 # Single sublevel, ordering is incorrect.
 check-ordering
@@ -158,4 +158,4 @@ L0.1
   a.SET.5-b.SET.6
   b.SET.7-d.SET.8
 ----
-L0.1 files 000003 and 000004 have overlapping ranges: [a#5,SET-b#6,SET] vs [b#7,SET-d#8,SET]
+L0.0 files 000001 and 000002 have overlapping ranges: [a#1,SET-b#2,SET] vs [b#1,SET-d#4,SET]

testdata/manual_compaction

@@ -110,7 +110,7 @@ range-deletions-bytes-estimate: 776
 
 # Same as above, except range tombstone covers multiple grandparent file boundaries.
 
-define target-file-sizes=(1, 1, 1, 1)
+define target-file-sizes=(1, 1, 1, 1) format-major-version=1
 L1
   a.SET.3:v
 L2