This change is 

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

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

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

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):

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):

Done.

Comments from Reviewable

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):

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

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):

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

Comments from Reviewable

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):

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

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):

Seek() on an invalid iter is fine.

Comments from Reviewable

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):

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

Comments from Reviewable