db: refactor table cache newIters

[jbowens]

This commit refactors the interface of the table cache newIters method to support creation of any of a sstable's iterators, including its point iterator, range deletion iterator and range key iterator. This is motivated by the refactor planned in #2863, which in turn requires refactoring some of the newIters call sites that determine whether ingested sstables overlap existing tables.

There's also a minor benefit that a compaction setup no longer needs to open point iterators and then immediately close them when initializing the range deletion iterators.

goos: linux
goarch: amd64
pkg: github.com/cockroachdb/pebble
cpu: Intel(R) Xeon(R) CPU @ 2.80GHz
              │ old-NewItersAlloc.txt │       new-NewItersAlloc.txt        │
              │        sec/op         │   sec/op     vs base               │
NewItersAlloc             427.0n ± 1%   412.1n ± 1%  -3.47% (p=0.000 n=10)

              │ old-NewItersAlloc.txt │     new-NewItersAlloc.txt      │
              │         B/op          │    B/op     vs base            │
NewItersAlloc              0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

              │ old-NewItersAlloc.txt │     new-NewItersAlloc.txt      │
              │       allocs/op       │ allocs/op   vs base            │
NewItersAlloc              0.000 ± 0%   0.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

[cockroach-teamcity]

This change is 

[sumeerbhola]

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @itsbilal and @jbowens)

---

table_cache.go line 166 at r1 (raw file):

	}
	if iter == nil {
		iter = emptyKeyspanIter

if the caller is going to place this in an iterSet can't we just return nil, since the iterSet handles the nil => non-nil empty iter transformation itself?

---

table_cache.go line 546 at r1 (raw file):

			// Return an empty iterator. This iterator has no mutable state, so
			// using a singleton is fine.
			return filteredAll, err

same question about letting iterSet handle the nil iter case.

---

table_cache.go line 1286 at r1 (raw file):

// bitwise-OR iterPointKeys, iterRangeDeletions and/or iterRangeKeys together to
// represent a set of desired iterator kinds.
type iterKinds int8

nit: how about uint8, given it is a bitmap.

[RaduBerinde]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @itsbilal and @jbowens)

---

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

// all returns an iterator set containing iterators over all the keys that meta
// contains.
func (f tableNewIters) all(

This method on a function is a bit dubious. I'd consider adding an iterAll constant and call it directly with that instead.

---

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

) (iterSet, error) {
	var kinds iterKinds
	if file.HasPointKeys {

[nit] Feels like this optimization belongs inside the implementation, not here

---

table_cache.go line 1248 at r1 (raw file):

}

// RangeDeletion returns the contained range deletion iterator. If there is no

[nit] Will something like HasRangeDeletion() bool be useful for fast paths?

[jbowens]

TFTRs!

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

---

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

Previously, RaduBerinde wrote…

This method on a function is a bit dubious. I'd consider adding an iterAll constant and call it directly with that instead.

Done.

---

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

Previously, RaduBerinde wrote…

[nit] Feels like this optimization belongs inside the implementation, not here

Done.

---

table_cache.go line 166 at r1 (raw file):

Previously, sumeerbhola wrote…

if the caller is going to place this in an iterSet can't we just return nil, since the iterSet handles the nil => non-nil empty iter transformation itself?

I removed this entire func; this was a remaining top-level table cache entry point that did not use iterSet in order to adapt to the function signature of keyspan.TableNewSpanIter. I replaced it with a new func tableNewRangeKeyIter that takes a newIters and returns a function with the appropriate signature.

---

table_cache.go line 546 at r1 (raw file):

Previously, sumeerbhola wrote…

same question about letting iterSet handle the nil iter case.

Added a comment here. This one is necessary because of the MaybeFilteredKeys() interface, but it will be go away with #2863.

---

table_cache.go line 1248 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] Will something like HasRangeDeletion() bool be useful for fast paths?

Hm, yeah, I think it might—I'll defer it for now but after #2863 we can look at the usages and adapt. Left a TODO

---

table_cache.go line 1286 at r1 (raw file):

Previously, sumeerbhola wrote…

nit: how about uint8, given it is a bitmap.

Good call, done.