storage: use RangeKeyChanged in ReadAsOfIterator

[erikgrinaker]

This patch uses RangeKeyChanged() to detect range keys in
ReadAsOfIterator, and caches them to improve performance.
It also fixes a bug where the iterator would fail to detect tombstones
with a non-empty MVCCValueHeader.

Resolves #84714.

Release justification: bug fixes and low-risk updates to new functionality

Release note: None

[cockroach-teamcity]

This change is 

[erikgrinaker]

@msbutler I was in the area anyway, so took the opportunity to fix this.

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @msbutler)

---

pkg/storage/read_as_of_iterator.go line 149 at r1 (raw file):

			f.newestRangeTombstone = hlc.Timestamp{}
			if hasRange {
				if v, ok := f.iter.RangeKeys().FirstBelow(f.asOf); ok {

I was surprised to find that FirstBelow is really FirstAtOrBelow. It might be worth renaming the method (ditto for FirstAbove) - I'm always worried about one-offs.

[tbg]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @msbutler)

[erikgrinaker]

bors r=tbg

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msbutler and @tbg)

---

pkg/storage/read_as_of_iterator.go line 149 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I was surprised to find that FirstBelow is really FirstAtOrBelow. It might be worth renaming the method (ditto for FirstAbove) - I'm always worried about one-offs.

Sure, will submit a separate PR in a bit.

[msbutler]

Thanks for doing this!!

Where exactly was the empty MVCC value header bug? Did it relate to how we used to detect tombstones len(f.iter.UnsafeValue()) == 0?

[msbutler]

conceptually, we need to pass seeked ==false to get info on f.newestRangeTombstone, since RangeKeyChanged() may be false, yet we still might have a rangekey to worry about, right?

Next time I make edits to this code, I might add a comment that explains this, or push this logic outside of advance and into readAsOfIterator.SeekGE to remove the parameter in advance.

[erikgrinaker]

Yes. Because we're getting an external iterator, whose state is unknown to us, the SeekGE call may land on a range key that the iterator was already positioned on, so we need to explicitly reset the state on any SeekGE call too.

[erikgrinaker]

Where exactly was the empty MVCC value header bug? Did it relate to how we used to detect tombstones len(f.iter.UnsafeValue()) == 0?

Correct. Although I suppose this wouldn't fire when we're reading from SSTs, where we currently only have empty value headers. If we add some though, e.g. the import job ID, this would fail.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)