storage: remove support for handling physically interleaved intents

[sumeerbhola]

Release note: None

[cockroach-teamcity]

This change is 

[erikgrinaker]

Thanks for looking this over, good catches as always.

We have a fair bit of other code related to interleaved intents too, would be good to remove it all. Fine with a separate PR though. For example:

cockroach/pkg/roachpb/api.proto

Lines 2078 to 2083 in 6be337e

	// ScanInterleavedIntentsRequest is the request for a ScanInterleavedIntents operation.
	// This is a read-only operation that returns all interleaved (non-separated)
	// intents found over the request range.
	message ScanInterleavedIntentsRequest {
	RequestHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
	}

cockroach/pkg/cli/debug.go

Line 1217 in 6314071

var interleavedIntentCount, separatedIntentCount int

cockroach/pkg/kv/db.go

Line 789 in 2675c7c

func (db *DB) ScanInterleavedIntents(

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens and @sumeerbhola)

---

pkg/storage/intent_interleaving_iter.go line 782 at r1 (raw file):

		// iter could now be at a bare range key that is equal to the intentIter
		// key.
		if err := i.maybeSkipIntentRangeKey(); err != nil {

Good catch, thanks. This seems problematic. Can you pull this out as a separate commit or PR and write a token GA-blocker issue for it, so we can backport to 22.2.0?

---

pkg/storage/intent_interleaving_iter_test.go line 185 at r1 (raw file):

}

// TODO(erikgrinaker,jackson): enhance TestIntentInterleavingIter with range

Is this not sufficiently covered by the TestMVCCHistories cases, which have a fair bit of intents in them?

[sumeerbhola]

TFTR!

Created #89428 to point to your list.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @jbowens)

---

pkg/storage/intent_interleaving_iter.go line 782 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

Good catch, thanks. This seems problematic. Can you pull this out as a separate commit or PR and write a token GA-blocker issue for it, so we can backport to 22.2.0?

I think it cannot occur in practice, so not really a GA-blocker.

• non-prefix iteration: even though pebble.Iterator.*WithLimit methods claim that the limit enforcement is best-effort, in practice it is not (to make the behavior deterministic for testing inside Pebble's metamorphic test). So if i.iter.Next stepped onto a new roachpb.Key, which is the only case where we could have a range but not a point, then i.intentIterState == pebble.IterAtLimit will also be true (since the previous limit would have been respected).
• prefix iteration: we cannot step onto a different roachpb.Key in i.iter.Next.

I made the change because I was worried about making an assumption about the Pebble implementation that may break in the future.
I'm a bit wary of backporting since when iterating over many MVCC versions of the same key, this extra call to maybeSkipIntentRangeKey can add cost, and don't want to add more performance work on top of what is already being done by folks for 22.2 regressions. Thoughts?

---

pkg/storage/intent_interleaving_iter_test.go line 185 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

Is this not sufficiently covered by the TestMVCCHistories cases, which have a fair bit of intents in them?

I took another look at TestMVCCHistories, and realized I had overlooked the iteration testing via the iter-* commands, which is quite comprehensive, so removed this todo.

[erikgrinaker]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jbowens and @sumeerbhola)

---

pkg/storage/intent_interleaving_iter.go line 782 at r1 (raw file):

Previously, sumeerbhola wrote…

I think it cannot occur in practice, so not really a GA-blocker.

• non-prefix iteration: even though pebble.Iterator.*WithLimit methods claim that the limit enforcement is best-effort, in practice it is not (to make the behavior deterministic for testing inside Pebble's metamorphic test). So if i.iter.Next stepped onto a new roachpb.Key, which is the only case where we could have a range but not a point, then i.intentIterState == pebble.IterAtLimit will also be true (since the previous limit would have been respected).
• prefix iteration: we cannot step onto a different roachpb.Key in i.iter.Next.

I made the change because I was worried about making an assumption about the Pebble implementation that may break in the future.
I'm a bit wary of backporting since when iterating over many MVCC versions of the same key, this extra call to maybeSkipIntentRangeKey can add cost, and don't want to add more performance work on top of what is already being done by folks for 22.2 regressions. Thoughts?

I see, thanks. In that case I agree, let's not much around with hot path backports this late in the game, and leave this for 23.1.

[sumeerbhola]

bors r=erikgrinaker

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)