mvcc: continue scanning if ReverseScan falls off end of range

[nvanbenschoten]

Fixes #17825.

When an mvccGetInternal call scans off the end of a key range
while attempting to find a value at a certain timestamp,
MVCCReverseScan shouldn't stop scanning backwards. This change
fixes this behavior by ignoring the iter.Valid in MVCCIterate
until after the iterator has scanned to the previous key.

[cockroach-teamcity]

This change is 

[tbg]

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/engine/mvcc.go, line 1742 at r1 (raw file):

				// The current entry is an inline value. We can reach the previous
				// entry using Prev() which is slightly faster than PrevKey().
				iter.Prev()

is it legal to call iter.Prev() on an iter that's not valid? Just checking. The "invalid iter" would have to keep enough state about where it is (i.e. "I'm at KeyMin" or "I'm at KeyMax"). We might have to re-seek, or add a comment explaining why it's all good.

ISTM that it would be illegal if valid == false but buf.meta.IsInline() == true. Something to clarify and perhaps assert.

---

pkg/storage/engine/mvcc_test.go, line 2321 at r1 (raw file):

	defer engine.Close()

	if err := MVCCPut(context.Background(), engine, nil, testKey2, hlc.Timestamp{WallTime: 1}, value2, nil); err != nil {

Could you add commentary narrating what's going on and what used to go wrong? I know what you're fixing, but it still takes me some energy to see through the random numbers.

---

Comments from Reviewable

[tbg]

mod comments.

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[bdarnell]

but I'm not sure about the answer to @tschottdorf's question.

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[nvanbenschoten]

TFTRs.

---

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/storage/engine/mvcc.go, line 1742 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

is it legal to call iter.Prev() on an iter that's not valid? Just checking. The "invalid iter" would have to keep enough state about where it is (i.e. "I'm at KeyMin" or "I'm at KeyMax"). We might have to re-seek, or add a comment explaining why it's all good.

ISTM that it would be illegal if valid == false but buf.meta.IsInline() == true. Something to clarify and perhaps assert.

Yeah, that's a good point. Before calling iter.Prev() on the inline case we should still check if the key is valid. If not, we should have the same behavior as before. Done.

---

pkg/storage/engine/mvcc_test.go, line 2321 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Could you add commentary narrating what's going on and what used to go wrong? I know what you're fixing, but it still takes me some energy to see through the random numbers.

Done.

---

Comments from Reviewable

[petermattis]

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/engine/mvcc.go, line 1742 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yeah, that's a good point. Before calling iter.Prev() on the inline case we should still check if the key is valid. If not, we should have the same behavior as before. Done.

I don't think iter.Prev() is legal on an iter that's not valid. The RocksDB comments say:

  // Moves to the previous entry in the source.  After this call, Valid() is
  // true iff the iterator was not positioned at the first entry in source.
  // REQUIRES: Valid()
  virtual void Prev() = 0;

Rather than trying to keep track of where the iterator is pointed, perhaps we should add a dummy "max" key when an engine is created.

PS Next() has the same requirement that iter.Valid() is true.

---

Comments from Reviewable

[tbg]

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/engine/mvcc_test.go, line 2321 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Done.

Thanks! 🎩 💵 (<- attempted "hat tip")

---

Comments from Reviewable

[nvanbenschoten]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/engine/mvcc.go, line 1742 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I don't think iter.Prev() is legal on an iter that's not valid. The RocksDB comments say:

  // Moves to the previous entry in the source.  After this call, Valid() is
  // true iff the iterator was not positioned at the first entry in source.
  // REQUIRES: Valid()
  virtual void Prev() = 0;

Rather than trying to keep track of where the iterator is pointed, perhaps we should add a dummy "max" key when an engine is created.

PS Next() has the same requirement that iter.Valid() is true.

The newest version of this change will only Seek() on an invalid iterator, never Prev() or PrevKey(). Are there any issues with this approach @petermattis?

---

Comments from Reviewable

[tbg]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/engine/mvcc.go, line 1742 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The newest version of this change will only Seek() on an invalid iterator, never Prev() or PrevKey(). Are there any issues with this approach @petermattis?

Seek() on an invalid iter is fine.

---

Comments from Reviewable

[petermattis]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/storage/engine/mvcc.go, line 1742 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Seek() on an invalid iter is fine.

The newest version looks fine. Nice job on tracking this down. Not at all where I expected the bug to be.

---

Comments from Reviewable