db: avoid loading later files for a level in SeekPrefixGE

[jbowens]

Don't load arbitrarily later files during levelIter.SeekPrefixGE. This avoids unnecessary IO and CPU overhead.

Close #3575.

[cockroach-teamcity]

This change is 

[RaduBerinde]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @itsbilal, @jbowens, and @sumeerbhola)

---

level_iter.go line 562 at r1 (raw file):

		// unnecessary. This has been observed in practice on slow shared
		// storage. See #3575.
		if l.prefix != nil && l.cmp(file.SmallestPointKey.UserKey[l.split(file.SmallestPointKey.UserKey):], l.prefix) > 0 {

The : looks in the wrong place here

[jbowens]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @RaduBerinde, and @sumeerbhola)

---

level_iter.go line 562 at r1 (raw file):

Previously, RaduBerinde wrote…

The : looks in the wrong place here

Ooof; fixed and added a test that I should've added to begin with

---

testdata/level_iter line 184 at r2 (raw file):

next
prev
prev

prevs removed because these are not actually permitted by the InternalIterator interface

[RaduBerinde]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @sumeerbhola)

---

level_iter.go line 562 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Ooof; fixed and added a test that I should've added to begin with

I would add a Prefix(userKey []byte) []byte method on base.Split and change this and the other callsite to l.split.Prefix(file.SmallestPointKey.UserKey).

[RaduBerinde]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @sumeerbhola)

---

level_iter.go line 562 at r1 (raw file):

Previously, RaduBerinde wrote…

I would add a Prefix(userKey []byte) []byte method on base.Split and change this and the other callsite to l.split.Prefix(file.SmallestPointKey.UserKey).

If you want, I can make that change separately, there are probably many other call sites that would benefit.

[jbowens]

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @RaduBerinde, and @sumeerbhola)

---

level_iter.go line 562 at r1 (raw file):

Previously, RaduBerinde wrote…

If you want, I can make that change separately, there are probably many other call sites that would benefit.

was working on it just now! pushed

[RaduBerinde]

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @itsbilal, @jbowens, and @sumeerbhola)

---

level_iter.go line 562 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

was working on it just now! pushed

Nice, thanks!

---

internal/base/comparer.go line 128 at r3 (raw file):

// Prefix returns the prefix of the key k, using s to split the key.
func (s Split) Prefix(k []byte) []byte {
	return k[:s(k)]

[nit] Do you see any downside to doing k[:n:n] just to be safe?

[sumeerbhola]

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @itsbilal and @jbowens)

---

level_iter.go line 563 at r2 (raw file):

		// storage. See #3575.
		if l.prefix != nil && l.cmp(file.SmallestPointKey.UserKey[:l.split(file.SmallestPointKey.UserKey)], l.prefix) > 0 {
			return noFileLoaded

Does returning noFileLoaded have any implication on the SeekGEFlags based optimizations?

[jbowens]

TFTR!

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @itsbilal, @RaduBerinde, and @sumeerbhola)

---

level_iter.go line 563 at r2 (raw file):

Previously, sumeerbhola wrote…

Does returning noFileLoaded have any implication on the SeekGEFlags based optimizations?

Not directly; added a comment discussing TrySeekUsingNext. Since the file hasn't been loaded, a subsequent seek with TrySeekUsingNext()=true will disable TrySeekUsingNext on the sstable iterator seek

---

internal/base/comparer.go line 128 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] Do you see any downside to doing k[:n:n] just to be safe?

I don't, done!

[sumeerbhola]

Reviewable status: 0 of 9 files reviewed, 5 unresolved discussions (waiting on @itsbilal, @jbowens, and @RaduBerinde)

---

level_iter.go line 563 at r5 (raw file):

		// storage. See #3575.
		if l.prefix != nil && l.cmp(l.split.Prefix(file.SmallestPointKey.UserKey), l.prefix) > 0 {
			// Note that because we don't update l.iter, a subsequent call to

nit: should this say "because l.iter will be nil, ..."

And there's no issue with the manifest.LevelIterator and the SeekGEFlags either because we've stopped earlier, yes?

[jbowens]

Reviewable status: 0 of 9 files reviewed, 5 unresolved discussions (waiting on @itsbilal, @RaduBerinde, and @sumeerbhola)

---

level_iter.go line 563 at r5 (raw file):

Previously, sumeerbhola wrote…

nit: should this say "because l.iter will be nil, ..."

And there's no issue with the manifest.LevelIterator and the SeekGEFlags either because we've stopped earlier, yes?

yes