db, sstable: disallow invalid empty keys in comparer functions

[RaduBerinde]

The empty key might or might not be valid depending on the Comparer
(it is valid with the DefaultComparer but not with the cockroach
comparer). However, the comparer was expected to handle empty keys
even if they were not valid.

This change fixes all code paths which end up passing an empty slice to
the comparer (except when it is expected that it is a valid key).

Key changes:

• cockroachkvs.Compare/Equal and testkeys.compare now panic under
  invariants if called with an empty key.
• Fix IsLowerBound in colblk data blocks to use ComparePointSuffixes
  instead of Compare when comparing bare suffixes.
• Internal code paths that could encounter nil keys (first iteration of
  a scan, uninitialized previous-key tracking, nil bounds) now guard
  against calling Compare with nil/empty keys by checking for nil first
  or using First() instead of SeekGE(nil).
• Fix overlap checks in flush/ingest to handle compaction bounds with
  nil Start or End.
• Fix determineExcisedTableSize for range-key-only tables that have no
  point keys and only a placeholder index entry.
• Various test fixes to avoid passing nil/empty keys to Compact or
  Compare.

[cockroach-teamcity]

This change is 

[annrpom]

@annrpom reviewed 33 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on sumeerbhola).

[sumeerbhola]

@sumeerbhola reviewed 21 files and made 8 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on RaduBerinde).

---

sstable/reader_iter_single_lvl.go line 561 at r1 (raw file):

		// Forward iteration.
		sep := PI(&i.index).Separator()
		if len(sep) == 0 || i.bpfs.boundLimitedFilter.KeyIsWithinUpperBound(sep) {

I am assuming that when the empty key is valid, sep can be zero length if the block only contains empty keys. And since we have a valid bound, the empty key must be contained in it. But can't the bound limited filter have a bound that is also the empty key, and so the block would not be excluded (as m.cmp(m.maskSpan.End, key) > 0 would be false)?
I am wondering if this is another reason to abandon empty keys.

---

sstable/reader_iter_single_lvl.go line 592 at r1 (raw file):

	} else {
		sep := PI(&i.index).Separator()
		if len(sep) > 0 && i.bpfs.boundLimitedFilter.KeyIsWithinLowerBound(sep) {

why a conjunction here and a disjunction in the previous case?

---

internal/base/internal.go line 475 at r1 (raw file):

	if len(k.UserKey) == 0 {
		if len(buf) == 0 {
			// The empty slice might not be a valid key, while buf is always valid.

I didn't understand this comment, specifically "buf is always valid".
We are making an InternalKey here that is valid only if the empty user key is valid, yes?

---

sstable/colblk/data_block.go line 372 at r1 (raw file):

func (ks *defaultKeySeeker) IsLowerBound(k []byte, syntheticSuffix []byte) bool {
	if len(k) == 0 {
		if invariants.Enabled {

is the empty key invalid here even if comparer supports it?

---

sstable/rowblk/rowblk_index_iter.go line 71 at r1 (raw file):

func (i *IndexIter) SeparatorLT(key []byte) bool {
	// An empty separator (from range-key-only sstables) is less than all
	// non-empty keys.

what if key is also empty?

---

compaction.go line 921 at r1 (raw file):

		c.maxOutputFileSize = uint64(opts.TargetFileSizes[0])
		c.maxOverlapBytes = maxGrandparentOverlapBytes(opts.TargetFileSizes[0])
		if c.bounds.Start != nil && c.bounds.End.Key != nil {

so no grandparents are calculated when one of these is nil? Can that happen when the empty key is valid?
What other causes for one of these to be nil?

---

compaction.go line 1794 at r1 (raw file):

			// to ingest-time splits or excises.
			ingestFlushable := c.flush.flushables[0].flushable.(*ingestedFlushable)
			if ingestFlushable.exciseSpan.Valid() {

I suppose this Valid call is an example of us not actually supporting empty keys since it is doing return k.Start != nil && k.End != nil (on master)?

Should we go one step further after this PR and say empty keys are not supported even if the Comparer can handle them?

---

compaction.go line 1805 at r1 (raw file):

					// write a file overlapping with the excise span.
					if bounds := c2.Bounds(); bounds != nil {
						if (bounds.Start == nil || exciseBounds.End.IsUpperBoundFor(d.cmp, bounds.Start)) &&

is this || to err on the side of cancellation (for safety)?

[sumeerbhola]

AI Review

Overall assessment: This is a well-scoped correctness PR. It systematically eliminates code paths that pass empty/nil keys to Compare/Equal, which panics under invariants with the cockroach comparer. The changes are categorized into: (1) invariant panics on invalid inputs, (2) nil guards before comparisons, (3) fixes for range-key-only sstable edge cases. The PR is sound with a few items to flag.

Issues

1. Minor: Typo in comment (sstable/rowblk/rowblk_iter.go ~line 705)

// UserKey could be an empty. The empty slice might or might not be a valid

Should be "UserKey could be empty."

2. Consider fixing Overlaps rather than reimplementing

The overlap check is manually reimplemented in both compaction.go:1802 and ingest.go:2466 to handle nil Start/End.Key:

if (bounds.Start == nil || exciseBounds.End.IsUpperBoundFor(d.cmp, bounds.Start)) &&
    (bounds.End.Key == nil || bounds.End.IsUpperBoundFor(d.cmp, exciseBounds.Start)) {

This duplicates UserKeyBounds.Overlaps logic. An alternative would be to make Overlaps (or a variant) handle nil bounds directly, avoiding the duplication. I understand this may be intentional to avoid changing the core API semantics (nil bounds meaning "unbounded" is a different contract), but it's worth considering.

3. IsLowerBound suffix comparison fix (sstable/colblk/data_block.go:382)

-	return ks.comparer.Compare(suffix, k[si:]) >= 0
+	return ks.comparer.ComparePointSuffixes(suffix, k[si:]) >= 0

This is a genuine bug fix independent of the empty-key theme. The original code was using Compare (which expects full keys with prefix+suffix) to compare bare suffixes. ComparePointSuffixes is the correct function. Worth calling out explicitly in the commit message since it's a semantically distinct fix.

4. Forward vs backward exclusion asymmetry (sstable/reader_iter_single_lvl.go)

For forward iteration with empty separator:

if len(sep) == 0 || i.bpfs.boundLimitedFilter.KeyIsWithinUpperBound(sep) {
    return blockExcluded
}

For backward iteration with empty separator:

if len(sep) > 0 && i.bpfs.boundLimitedFilter.KeyIsWithinLowerBound(sep) {

The asymmetry is: forward excludes blocks with empty separators, backward falls through to blockIntersects (loads the block). Both are safe for range-key-only sstables since the data block is empty anyway, but the forward case is slightly more efficient. This seems intentional and correct — just noting the asymmetry.

5. rowblk_iter.go SeekGE — empty UserKey is never returned

The new logic ensures entries with empty UserKey are always skipped:

if len(i.ikv.K.UserKey) > 0 && i.cmp(i.ikv.K.UserKey, key) >= 0 {

This is correct for the stated scenario (placeholder entries in range-key-only sstables), but the invariant that "empty UserKey = invalid/placeholder entry" isn't explicitly documented. A brief comment would help.

Looks Good

• cockroachkvs.Compare/Equal and testkeys.compare panics: Correctly placed inside the existing len(a) == 0 || len(b) == 0 guard, with the non-invariant fallback preserved.
• db.Compact nil rejection: Good API hardening. Uses errors.Errorf correctly.
• ScanStatistics nil guard (db.go:2799): prevKey.UserKey != nil && correctly handles the first iteration.
• Iterator.SetBounds/SetOptions: The i.opts.LowerBound == nil || short-circuit correctly avoids calling equal(nil, nil).
• determineExcisedTableSize early return for range-key-only tables: Size = 1 matches the existing fallback behavior at the bottom of the function and prevents division-by-zero.
• UserKeyBounds.Valid nil check: Correct — IsUpperBoundFor would panic on nil Start.
• DataBlockValidator.Validate restructuring: Moving Compare/obsolete checks inside if i > 0 is correct; for i==0 prevKey is uninitialized.
• exciseSpan.Valid() guard in compaction.go: Correctly skips the overlap loop when there's no excise span.
• flushable.go First() fallback: Using First() instead of SeekGE(nil) when bounds.Start is nil is the right approach.
• level_checker.go nil guards: All three additions correctly handle the uninitialized-first-iteration pattern.
• Test changes: Mechanical nil → []byte{0} or []byte{} replacements are correct.
• ParseTableMetadataDebug (table_metadata.go): Adding m.HasPointKeys && guard prevents comparing against uninitialized point key bounds.

[RaduBerinde]

@RaduBerinde made 4 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on sumeerbhola).

---

compaction.go line 1794 at r1 (raw file):

Previously, sumeerbhola wrote…

I suppose this Valid call is an example of us not actually supporting empty keys since it is doing return k.Start != nil && k.End != nil (on master)?

Should we go one step further after this PR and say empty keys are not supported even if the Comparer can handle them?

I am considering allowing empty keys but not nil keys - valid keys would have to be not nil even if they are empty.

I started by disallowing empty keys in all cases.. but I hesitated because RocksDB supported them and our existing formats since also support them. But I wouldn't be surprised if there are bugs related to empty keys, and tracking all of them down would be a waste of time for us..

---

sstable/reader_iter_single_lvl.go line 561 at r1 (raw file):

Previously, sumeerbhola wrote…

I am assuming that when the empty key is valid, sep can be zero length if the block only contains empty keys. And since we have a valid bound, the empty key must be contained in it. But can't the bound limited filter have a bound that is also the empty key, and so the block would not be excluded (as m.cmp(m.maskSpan.End, key) > 0 would be false)?
I am wondering if this is another reason to abandon empty keys.

Good point (although I think it applies to the lower bound case below - here the empty key would inherently pass any upper bound).

What I don't quite understand is why do we see empty separators when empty keys are not valid.. I'll try to understand it some more.

---

sstable/reader_iter_single_lvl.go line 592 at r1 (raw file):

Previously, sumeerbhola wrote…

why a conjunction here and a disjunction in the previous case?

Because the empty key is the smallest key.. But indeed here we have a problem if the empty key does pass the lower bound.

---

sstable/colblk/data_block.go line 372 at r1 (raw file):

Previously, sumeerbhola wrote…

is the empty key invalid here even if comparer supports it?

Good catch. I think it's left over from a version where I disallowed it for all comparers.