modulo small comment about the extra allocation caused by transforming the pointer to a slice.

Reviewed 34 of 34 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/storage/batcheval/cmd_get.go, line 47 at r2 (raw file):

	var intents []roachpb.Intent
	if intent != nil {
		intents = append(intents, *intent)

This results in an extra allocation as we allocated once to return the *intent and again here to create the backing array for the slice. Is this worth worry about? Probably not.

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

	// MVCCGet is the internal implementation of the family of package-level
	// MVCCGet functions. The notable difference is that key/value pairs are
	// returned raw, as a buffer of varint-prefixed slices, alternating from key

This doesn't look accurate. Iterator.MVCCGet has the same interface as the package-level MVCCGet.

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

// ...
// keyA_Timestamp_0 : value of version_0
// keyB : MVCCMetadata of keyB

Is there a documentation comment elsewhere about the MVCC key structure? Seems a shame to remove this if this is the only comment describing the structure.

pkg/storage/engine/rocksdb.go, line 2257 at r1 (raw file):

	state := C.MVCCGet(
		r.iter, goToCSlice(key), goToCTimestamp(timestamp),
		goToCTxn(opts.Txn), C.bool(!opts.Inconsistent), C.bool(opts.Tombstones),

I wonder if you should go the extra mile and change the libroach MVCCGet function parameter from consistent to inconsistent. It will be a bit confusing in the future to stumble across that polarity change.

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

pkg/storage/batcheval/cmd_get.go, line 47 at r2 (raw file):

Yeah, I figured it was safe because nothing perf-sensitive uses the Get API AFAIK. (E.g., result.FromIntents allocates again to construct the result.)

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

Oops, thanks for the fact check. Done.

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

Good point. Not that I can find. I've moved the diagram to the doc.go in this package.

pkg/storage/engine/rocksdb.go, line 2257 at r1 (raw file):

Yeah, I'd been thinking about that. Done. Thanks for the push.

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

pkg/storage/batcheval/cmd_get.go, line 47 at r2 (raw file):

Yeah, I'm ok with this.