The plumbing of usePrefixIter . I'm not familiar with CollectIntentRows and its usage, though, so it might be worthwhile for someone else to give this change a look as well.

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

pkg/kv/kvserver/batcheval/intent.go, line 41 at r1 (raw file):

// TODO(nvanbenschoten): remove the usePrefixIter complexity when we're fully on
// Pebble and can guarantee that all iterators created from a read-only engine
// are consistent.

Note that there is additional work needed in Pebble to make this work. Specifically, we need to add an Iterator.Clone() method. Doing so should be straightforward. I mention this only because right now using Pebble would encounter this same problem.

TFTR! There's no one who is particularly familiar with CollectIntentRows because it hasn't been touched in years.

@irfansharif do you mind giving this a pass as well? There are only three callers of CollectIntentRows and only two uses of roachpb.READ_UNCOMMITTED.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @petermattis)

pkg/kv/kvserver/batcheval/intent.go, line 41 at r1 (raw file):

Yeah, I mentioned that in the TODO in engine.go. It seemed too specific for this TODO though.

Reviewed 6 of 7 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @petermattis)

pkg/kv/kvserver/batcheval/intent.go, line 80 at r1 (raw file):

			return roachpb.KeyValue{}, err
		} else if val == nil {
			return roachpb.KeyValue{}, errors.Errorf("provisional value for intent %+v not found", intent)

This seems like a change in behavior. Before it seems like we used to just skip the intent. Is it the case that actually val is never nil and instead val.Value is empty on deletes and you're fixing an old bug?

Should we add testing for this case? When can it happen?

LGTM.

Reviewed 7 of 7 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @nvanbenschoten, and @petermattis)

pkg/kv/kvserver/batcheval/intent.go, line 79 at r1 (raw file):

		if err != nil {
			return roachpb.KeyValue{}, err
		} else if val == nil {

nit: was clearer without the else. Could just be an isolated if.

pkg/kv/kvserver/batcheval/intent.go, line 80 at r1 (raw file):

Ditto.

pkg/kv/kvserver/batcheval/intent.go, line 84 at r1 (raw file):

		return roachpb.KeyValue{Key: intent.Key, Value: *val}, nil
	}
	res, err := storage.MVCCScanAsTxn(

It seems that MVCCScanAsTxn uses the non-prefix iterator. For my own curiosity, where is the code for that? (what I really want to know is the when one is chosen over the other)

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

// should rarely be used.
//
// The read is carried out without the chance of uncertainty restarts.

Here and MVCCGetAsTxn, is it worth asserting on this?

It was L1, and I just rotated off.

Ah, sorry, I thought you were ahead of me in the KV L2 rotation, so that's where my mind jumped to.

Thanks for the two reviews!

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

pkg/kv/kvserver/batcheval/intent.go, line 79 at r1 (raw file):

Done here and below.

pkg/kv/kvserver/batcheval/intent.go, line 80 at r1 (raw file):

This is a good catch! I was thinking that the previous code was unnecessary because we would never even see a deletion intent in the first place, but that's incorrect. We'll see the intent when scanning non-transactionally, but then we'll get a nil roachpb.Value from MVCCGet because we're not scanning for tombstones. I've revived the earlier logic and added tests for all of these cases.

pkg/kv/kvserver/batcheval/intent.go, line 84 at r1 (raw file):

That's right at the top of MVCCScan and MVCCGet. Notice how theIterOptions they provide to reader.NewIterator differ.

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

Done at the same place I added the assertion against WriteIntentErrors.

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif and @petermattis)

pkg/kv/kvserver/batcheval/intent_test.go, line 113 at r2 (raw file):

	for _, c := range testCases {
		t.Run(c.name, func(t *testing.T) {
			// Test with and without deletion intents. If a READ_UNCOMMITTED request

These semantics seem potentially surprising and probably deserve to be documented somewhere if they aren't already. Is it correct that if you run a read in READ_UNCOMMITTED and there's an uncommitted deletion we'll return the current value?

This is definitely not for this PR though. Always interesting to learn about the edge cases in the bespoke, rarely used modes like READ_UNCOMMITTED

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @ajwerner and @petermattis)

pkg/kv/kvserver/batcheval/intent_test.go, line 113 at r2 (raw file):

These semantics seem potentially surprising and probably deserve to be documented somewhere if they aren't already. Is it correct that if you run a read in READ_UNCOMMITTED and there's an uncommitted deletion we'll return the current value?

Yes, that is true. So we probably should be returning deletion tombstones in the IntentRows slice to indicate that the committed value is being deleted. I added a comment to the documentation on these fields.