storage: add MVCCIterKind parameter to Reader functions

[sumeerbhola]

Also changes all callers to try to appropriately set these parameters.
Note that MVCCKeyAndIntentsIterKind would always be correct, but
where possible, we want to set MVCCKeyIterKind.

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 17 of 17 files at r1, 44 of 44 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @itsbilal, @petermattis, and @sumeerbhola)

---

pkg/kv/kvserver/replica_consistency.go, line 578 at r2 (raw file):

	// Iterate over all the data in the range.
	iter := snap.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{UpperBound: desc.EndKey.AsRawKey()})

This raises an interesting question. At first, I thought that this was incorrect because we're already walking over each part of the range in down below (see call to MakeReplicatedKeyRanges). But I think the call to ComputeStatsGo does want to see the intents merged into the MVCC keyspace. So should we not call ComputeStatsGo on the two lock-table key spans?

---

pkg/kv/kvserver/replica_evaluate.go, line 92 at r2 (raw file):

		return origReqs
	}
	iter := reader.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{

I think we can skip intents here. We're just looking for anything in the key range.

---

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

	// engine. The caller must invoke MVCCIterator.Close() when finished
	// with the iterator to free resources.
	NewMVCCIterator(iterKind MVCCIterKind, opts IterOptions) MVCCIterator

Is it safe to assume that you didn't want to put MVCCIterKind into IterOptions because you wanted to force each caller to specify it?

---

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

	txn *roachpb.Transaction,
) error {
	iter := newMVCCIterator(rw, timestamp == (hlc.Timestamp{}), IterOptions{Prefix: true})

hlc.Timestamp does have an IsEmpty method if you want to use that.

[sumeerbhola]

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @itsbilal, @nvanbenschoten, and @petermattis)

---

pkg/kv/kvserver/replica_consistency.go, line 578 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This raises an interesting question. At first, I thought that this was incorrect because we're already walking over each part of the range in down below (see call to MakeReplicatedKeyRanges). But I think the call to ComputeStatsGo does want to see the intents merged into the MVCC keyspace. So should we not call ComputeStatsGo on the two lock-table key spans?

Glad you noticed that. I've added a TODO, since MakeReplicatedKeyRanges does not currently include any of the lock table ranges.

---

pkg/kv/kvserver/replica_evaluate.go, line 92 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think we can skip intents here. We're just looking for anything in the key range.

Done

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is it safe to assume that you didn't want to put MVCCIterKind into IterOptions because you wanted to force each caller to specify it?

Exactly. It is less likely to cause bugs now or in the future.

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

hlc.Timestamp does have an IsEmpty method if you want to use that.

Done

[sumeerbhola]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)