Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/storage/engine/enginepb/mvcc.proto, line 78 at r1 (raw file):

  // returns the value at a given sequence.
  // History will be empty for non-transactional requests.
  optional IntentHistory history = 8 [(gogoproto.nullable) = false];

Adding non-nullable optional fields to below-raft protos is not generally safe. Repeated fields are fine, though, so I think you can just remove the IntentHistory wrapper.

Haven't gotten to the tests yet, but this is looking good so far.

Reviewed 9 of 12 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/storage/engine/mvcc.go, line 1116 at r2 (raw file):

}

// getExistingValue returns the value of the data read at readTS from the buffer pool.

Let's inline this instead of pulling it out. We do so in mvccResolveWriteIntent and it doesn't seem to be a problem.

pkg/storage/engine/mvcc.go, line 1272 at r2 (raw file):

			cleanup()

			// TODO(ridwanmsharif): Confirm this is correct.

Yep 👍

pkg/storage/engine/mvcc.go, line 1277 at r2 (raw file):

			// version.  For example, a conditional put within same
			// transaction should read previous write.
			if value, err = maybeGetValue(

Can we avoid reading the previous intent value twice?

pkg/storage/engine/enginepb/mvcc.go, line 134 at r2 (raw file):

// AddToHistory adds the sequence and value to the intent history.
func (meta *MVCCMetadata) AddToHistory(seq int32, val []byte, deleted bool) {

s/AddToHistory/AddToIntentHistory/

pkg/storage/engine/enginepb/mvcc.proto, line 27 at r2 (raw file):

// tombstone - to be stored in an IntentHistory of a key during a transaction.
// Making it possible for a transaction to be more idempotent
message Entry {

This name should be more specific. What does enginepb.Entry mean? I'd make it a nested message within MVCCMetadata and give it a more specific name. Maybe SequencedIntent.

pkg/storage/engine/enginepb/mvcc.proto, line 33 at r2 (raw file):

  // IntentHistory.GetValue(seq) returns the value
  // in history if it exists.

This comment about a method on MVCCMetadata doesn't belong here.

pkg/storage/engine/enginepb/mvcc.proto, line 39 at r2 (raw file):

  optional bytes value = 2;
  // Is this value in the intent history a deletion tombstone?
  optional bool deleted = 3 [(gogoproto.nullable) = false];

Does this need to be separate from value == nil?

pkg/storage/engine/enginepb/mvcc.proto, line 69 at r2 (raw file):

  optional bytes raw_bytes = 6;
  // IntentHistory of the transaction stores all they values the txn wrote
  // for the key along with each values corresponding Sequence.

Not "all" versions, just all non-live versions, right?

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/storage/engine/mvcc.go, line 1116 at r2 (raw file):

Done.

pkg/storage/engine/mvcc.go, line 1272 at r2 (raw file):

Done.

pkg/storage/engine/mvcc.go, line 1277 at r2 (raw file):

Done.

pkg/storage/engine/enginepb/mvcc.go, line 134 at r2 (raw file):

Done.

pkg/storage/engine/enginepb/mvcc.proto, line 78 at r1 (raw file):

Done. The empty checksum has not changed but the populated checksum for the below raft protos test did, which is what we want I think.

pkg/storage/engine/enginepb/mvcc.proto, line 27 at r2 (raw file):

Done.

pkg/storage/engine/enginepb/mvcc.proto, line 33 at r2 (raw file):

Done.

pkg/storage/engine/enginepb/mvcc.proto, line 39 at r2 (raw file):

It doesn't, I'd misunderstood what the Deleted bool on the MVCCMetadata object was doing. Fixed.

pkg/storage/engine/enginepb/mvcc.proto, line 69 at r2 (raw file):

Done.

Almost there. The testing is solid.

Reviewed 9 of 10 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/storage/engine/mvcc.go, line 1221 at r3 (raw file):

	var maybeTooOldErr error
	var prevValSize int64
	var prevValBytes []byte

These are referring to two different "prev vals", right? One is the previous intent value is one is the previous committed value. I'd rename prevValBytes to prevIntentValBytes and prevSequence to prevIntentSequence.

Also, move this into the ok && meta.Txn != nil block since we only need it there.

pkg/storage/engine/mvcc.go, line 1255 at r3 (raw file):

				return err
			}
			if existingVal != nil {

Can this case exist? If not, should we throw an error?

pkg/storage/engine/mvcc.go, line 1259 at r3 (raw file):

			}
			prevSequence := meta.Txn.Sequence
			// Make sure we process valueFn before clearing any earlier

minor nit: add a line break above this comment.

pkg/storage/engine/mvcc.go, line 1268 at r3 (raw file):

				}
			}
			// Release the buffer after using the existing value.

We're leaking this in two different places. I'd defer it above.

pkg/storage/engine/mvcc.go, line 1305 at r3 (raw file):

				timestamp = metaTimestamp
			}
			// Since a transaction exists for the MVCCMetadata, we must add the

Not just any transaction, our transaction. I'd rephase to "since an intent with a smaller sequence number for the same transaction exists, we must ..."

pkg/storage/engine/mvcc.go, line 1307 at r3 (raw file):

			// Since a transaction exists for the MVCCMetadata, we must add the
			// previous value and sequence to the intent history.
			meta.AddToIntentHistory(prevSequence, prevValBytes)

I'm a little worried about modifying the meta directly. Can we avoid that?

pkg/storage/engine/mvcc_test.go, line 4384 at r3 (raw file):

// TestMVCCIntentHistory verifies that trying to write to a key that already was written
// to - results in the history being recorded in the MVCCMetadata.

Why the dash?

pkg/storage/engine/mvcc_test.go, line 4385 at r3 (raw file):

// TestMVCCIntentHistory verifies that trying to write to a key that already was written
// to - results in the history being recorded in the MVCCMetadata.
func TestMVCCIntentHistory(t *testing.T) {

Nice test!

pkg/storage/engine/enginepb/mvcc.proto, line 69 at r2 (raw file):

Add another sentence stating that it doesn't include the latest intent on the key, just those that have been overwritten.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/storage/engine/mvcc.go, line 1221 at r3 (raw file):

Done.

pkg/storage/engine/mvcc.go, line 1255 at r3 (raw file):

So currently this can happen when a transaction encounters an intent with a lower epoch. As a result it can't read the value using mvccGetInternal. I added a comment here but essentially, this shouldn't be a problem because in this case, the intent history is blown away anyway.

pkg/storage/engine/mvcc.go, line 1268 at r3 (raw file):

Done.

pkg/storage/engine/mvcc.go, line 1305 at r3 (raw file):

Done.

pkg/storage/engine/mvcc.go, line 1307 at r3 (raw file):

Done. Updating buf.newMeta instead.

pkg/storage/engine/mvcc_test.go, line 4384 at r3 (raw file):

Done.

pkg/storage/engine/mvcc_test.go, line 4385 at r3 (raw file):

Done.

pkg/storage/engine/enginepb/mvcc.proto, line 69 at r2 (raw file):

Done.

once the final two comments are addressed. Nice work on this!

Reviewed 1 of 4 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/storage/engine/mvcc.go, line 1255 at r3 (raw file):

Consider asserting that.

pkg/storage/engine/mvcc.go, line 1376 at r4 (raw file):

		}
		buf.newMeta.Txn = txnMeta
		buf.newMeta.Timestamp = hlc.LegacyTimestamp(timestamp)

Didn't we already do this above?

Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/storage/engine/enginepb/mvcc.proto, line 78 at r1 (raw file):

Yes, that's right.

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/storage/engine/mvcc.go, line 1255 at r3 (raw file):

Done.

pkg/storage/engine/mvcc.go, line 1376 at r4 (raw file):

Done.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/storage/engine/mvcc.go, line 1321 at r5 (raw file):

			// If the epoch of the transaction doesn't match the epoch of the
			// intent, blow away the intent history.
			if txn.Epoch <= meta.Txn.Epoch {

Do we have any tests for txn.Epoch < meta.Txn.Epoch? Don't we already return an error in that case?

pkg/storage/engine/mvcc.go, line 1322 at r5 (raw file):

			// intent, blow away the intent history.
			if txn.Epoch <= meta.Txn.Epoch {
				// I don't know where this case could pop up, but it is worth

nit: Comments are generally not in the first person.

pkg/storage/engine/mvcc.go, line 1326 at r5 (raw file):

				// to the history
				if existingVal == nil {
					return errors.Errorf("previous intent could not be read")

Leave a bit more context here. For instance, take a look at some of the warnings in mvccResolveWriteIntent.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/storage/engine/mvcc.go, line 1321 at r5 (raw file):

Done. Should've been ==

pkg/storage/engine/mvcc.go, line 1326 at r5 (raw file):

Done.

Reviewable status: complete! 1 of 0 LGTMs obtained