sstable: fix twoLevelIterator SeekGE optimization

[sumeerbhola]

The current TrySeekUsingNext optimization had a bug where a first SeekGE could set exhaustedBounds=+1, and the next
monotonic twoLevelIterator.SeekGE (without changing bounds) would set exhaustedBounds back to 0, call singleLevelIterator.SeekGE which would immediately return because of i.data.isDataInvalidated() (and TrySeekUsingNext), which would cause the twoLevelIterator to step the top-level index iterator.
This woud break a subsequent optimization when the bounds were monotonically advanced, since the top-level index iterator had been advanced too far. See #2036 (comment) for an example of the bug.

Given that both singleLevelIterator and twoLevelIterator use the exhaustedBounds value from the previous SeekGE call for the TrySeekUsingNext optimization, the twoLevelIterator should be selective on when it resets exhaustedBounds to 0. The logic behind when this can be done is not complex, so should be maintainable.

This bug should only occur when using block property filters with the monotonic bound optimization: the second SeekGE in the above, which moves the top-level index too far forward would not happen if the singleLevelIterator was actually loading data blocks, since the singleLevelIterator would do some work and set exhaustedBound back to +1.

The PR also constrains this TrySeekUsingNext fast path to the case where i.err == nil. This was an oversight, though very unlikely to happen in practice.

There are todos added to make further changes after we backport this change:

• The twoLevelIterator.SeekPrefixGE code does not currently utilize TrySeekUsingNext. There is no reason to hold back on this optimization.
• The cases where the twoLevelIterator is already exhausted are not optimized. We will wastefully reseek the top level index.

Fixes #2036

[cockroach-teamcity]

This change is 

[sumeerbhola]

I owe some unit tests for this fix. This fixes the specific metamorphic test failure. I'll let stressmeta run for a few hours.

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @jbowens)

[sumeerbhola]

I added some longer commentary about our iterator optimizations and cleaned up some invariant checking. I am feeling more confident about this now. Could use a quick look while I work on adding unit tests.

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @jbowens)

[sumeerbhola]

Added a unit test

[sumeerbhola]

Metamorphic testing with the temporary changes mentioned in #2059 was ok for 16+ hours, but failed without this fix (the failure mentioned in that issue).

[jbowens]

Reviewed 2 of 4 files at r1, 8 of 11 files at r2, 5 of 6 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sumeerbhola)

---

sstable/reader.go line 1692 at r4 (raw file):

		// This invariant checking is important enough that we do not gate it
		// behind invariants.Enabled.
		if true {

Is this if true block temporary?

---

sstable/reader.go line 1694 at r4 (raw file):

		if true {
			if i.boundsCmp > 0 {
				flags = flags.DisableTrySeekUsingNext()

should we leave a TODO to fix TestIterRandomizedMaybeFilteredKeys?

[sumeerbhola]

TFTR!

Reviewable status: 16 of 17 files reviewed, 2 unresolved discussions (waiting on @jbowens)

---

sstable/reader.go line 1692 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Is this if true block temporary?

Removed

---

sstable/reader.go line 1694 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

should we leave a TODO to fix TestIterRandomizedMaybeFilteredKeys?

Added a TODO

[sumeerbhola]

Cannot reproduce the TestCompactionTombstones race failure. Ignoring.