storage: Enforce that all keys accessed are declared in SpanSet

[bdarnell]

Adds DeclareKeys implementations for all commands. Does not yet cover
keys whose use is implied by use of in-memory fields of the replica
state.

This is an updated and cleaned-up version of #10084.

---

This change is 

[petermattis]

---

Reviewed 6 of 6 files at r1, 5 of 5 files at r2.
Review status: 8 of 13 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/storage/span_set.go, line 99 at r2 (raw file):

type spanSetIterator struct {
	i     engine.Iterator

I think if you embedded engine.Iterator, I think you can avoid some of the method exporting you did in the previous commit. I don't feel particularly strongly about this, though.

---

Comments from Reviewable

[bdarnell]

Review status: 8 of 13 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/storage/span_set.go, line 99 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think if you embedded engine.Iterator, I think you can avoid some of the method exporting you did in the previous commit. I don't feel particularly strongly about this, though.

I think only the visible methods of an embedded field get promoted to the new struct. More importantly, if this were embedded, the compiler would no longer warn us if we added a new method to the interface without implementing it here.

---

Comments from Reviewable

[petermattis]

Review status: 8 of 13 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/storage/span_set.go, line 99 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I think only the visible methods of an embedded field get promoted to the new struct. More importantly, if this were embedded, the compiler would no longer warn us if we added a new method to the interface without implementing it here.

I'm pretty sure all of the methods, not just the exported ones, get promoted to the new struct. We used this in sql-land at some point. I think we also used this in rangeDataIterator in some incarnation. Regardless, the current approach is fine.

---

Comments from Reviewable

[bdarnell]

Added some tests and fixed some bugs (a couple in the new spanSetIterator code, and one in the unused roachpb.Span.Contains). Will squash the last commit before merging.

[petermattis]

---

Reviewed 5 of 5 files at r3, 2 of 2 files at r4.
Review status: 13 of 15 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/storage/span_set.go, line 111 at r4 (raw file):

func (s spanSetIterator) Seek(key engine.MVCCKey) {
	if s.err == nil {

What's this about? The previous code was only overwriting s.err if it was nil. Was that problematic?

---

pkg/storage/span_set_test.go, line 66 at r5 (raw file):

	ss.Add(SpanReadOnly, roachpb.Span{Key: roachpb.Key("g")})
	ss.Add(SpanReadOnly, roachpb.Span{Key: roachpb.Key("k"), EndKey: roachpb.Key("q")})
	t.Logf("%s", ss.spans)

Remove?

---

Comments from Reviewable

[bdarnell]

Review status: 13 of 15 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/storage/span_set.go, line 111 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

What's this about? The previous code was only overwriting s.err if it was nil. Was that problematic?

In the first version of this change, an invalidated iterator stays invalid forever. With this line removed, an iterator can be reused by seeking back in bounds. I don't know how much we care about that but it seemed like the right thing to do.

---

pkg/storage/span_set_test.go, line 66 at r5 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Remove?

Done.

---

Comments from Reviewable

[bdarnell]

I forgot to test the previous round of this PR with propEvalKV enabled (which is the only time it does anything interesting), so I had to make some more updates. Unfortunately even though I uploaded all my intermediate commits before rebasing and squashing, reviewable seems to have eaten them so it's not easy to review just what changed in this round. Fortunately there's nothing too interesting in the new parts. There's a change to SpanSetIterator semantics (stepping out of bounds sets Valid() to false, but isn't an error), additions of new keys to various declareKeys methods, and a bit of a hack to unwrap the SpanSetIterator in storageccl.WriteBatch.

---

Review status: 4 of 16 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable