storage/gc: create gc pkg, reverse iteration in rditer, paginate versions during GC

[ajwerner]

This PR comes in 6 commits. See the commit messages for more details. The meat of the change is in: storage: rework RunGC so it no longer buffers keys and values in memory.

1. Fix the spanset Iterator such that SeekLT can seek to the end of the range.
2. Expose reverse iteration in rditer.ReplicaDataIterator.
3. Rearrange some code in gc_queue.go to put the RunGC() helpers below that function.
4. Remove lockableGCInfo because there was no need for it.
5. Move RunGC into a gc subpackage.
6. Rework RunGC to paginate versions of keys with reverse iteration.

Fixes #42531.

[cockroach-teamcity]

This change is 

[ajwerner]

The last commit needs more testing and probably a benchmark but I wanted to put it up early because I suspect it's going to be at least a little bit controversial. On the whole I feel that it makes the code easier to read but I'd like some confirmation of that feeling.

[ajwerner]

Hey @tbg I hear you're coming back soon. This PR is certainly not ready to merge but I'd appreciate if you could give the 5th commit a look to judge the basic approach. I want to make sure the structure is reasonable before I invest in the testing and benchmarking to get this over the finish line.

[tbg]

Glad to have you in this area of the code! The changes look very good. I'm not too worried that things will have slowed down due to the reverse iteration, but others will have more informed opinions on that.
The comments I left are mostly smaller suggestions to clarify the code. I have held off on reviewing some core pieces of logic pending your next round of changes.

Reviewed 4 of 4 files at r1, 7 of 7 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 4 of 4 files at r5, 1 of 1 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/storage/batch_spanset_test.go, line 193 at r1 (raw file):

		t.Fatalf("expected valid iterator, err=%v", err)
	}
	// SeekLT the upper bound is invalid.

Lower bound, right? Span set has [c,g), and insideKey==c.

---

pkg/storage/gc_iterator.go, line 40 at r5 (raw file):

// this case, next is the value of an intent.
func (it *gcIterator) state() (
	cur, next *engine.MVCCKeyValue,

This is dangerously close to warranting a struct that is being returned. You're often returning mostly zero values, so it would actually help readability quite a bit if you introduced

type gcIterState struct {
  cur, next *engine.MVCCKeyValue
  isNewest, isIntent, ok bool
  err error

It would also give you a more convenient place for comments.
Is ok really needed? A comment explaining what it does would help.

I'll assume this method does the right thing for now, will revisit in post your next round of changes.

---

pkg/storage/engine/gc.go, line 36 at r5 (raw file):

}

// IsGarbage makes a determination whether a key (cur) is garbage. Next, if

Write 'next' here to make this easier to parse.

---

pkg/storage/engine/gc.go, line 37 at r5 (raw file):

non-nil,

s/next/newer/

---

pkg/storage/engine/gc.go, line 40 at r5 (raw file):

// metadata KV if cur is an intent). If isNewest is false, next must be non-nil.
// isNewest implies that this is the highest timestamp committed version for
// this key. If isNewest is true and next is non-nil, it is an intent.

Explain that there being an intent doesn't matter in the impl below. Conservatively we have to assume that the intent will get aborted, so we will be able to GC just the values that we could remove if there weren't an intent. Maybe this comment doesn't have to live in the method comment, but it would be helpful somewhere.

---

pkg/storage/engine/gc_test.go, line 13 at r5 (raw file):

package engine

import (

Do these tests need to be replaced?

---

pkg/storage/spanset/spanset.go, line 185 at r1 (raw file):
Can you add a comment for spanKeyExclusive? Something like

If spanKeyExclusive is set, the spans in access will be made open, i.e. [a,b) will be considered (a,b).

---

pkg/storage/spanset/spanset.go, line 194 at r1 (raw file):

		for _, cur := range s.spans[ac][scope] {
			if cur.Contains(span) &&
				(!spanKeyExclusive || cur.EndKey != nil && !cur.Key.Equal(span.Key)) ||

Is this EndKey check in the right place? I think this should be

!spanKeyExclusive || !cur.Key.Equal(span.Key) || (cur.EndKey != nil && cur.Key.Equal(span.EndKey))

---

pkg/storage/spanset/spanset.go, line 195 at r1 (raw file):

			if cur.Contains(span) &&
				(!spanKeyExclusive || cur.EndKey != nil && !cur.Key.Equal(span.Key)) ||
				spanKeyExclusive && cur.EndKey.Equal(span.Key) {

Isn't spanKeyExclusive always true if this expr is evaluated?

Having read the tests I think what's happening here is that cur = [k1, k2) is treated like (k1, k2]. I find the bool is not named intuitively for that behavior. Could "reverse" (along with a comment) be a better name?

---

pkg/storage/spanset/spanset.go, line 225 at r1 (raw file):

		for _, cur := range s.spans[ac][scope] {
			if (cur.Contains(span) &&
				(!spanKeyExclusive || (cur.EndKey != nil && !cur.Key.Equal(span.Key)))) ||

Similarly confused here

---

pkg/storage/spanset/spanset_test.go, line 224 at r1 (raw file):

	defer leaktest.AfterTest(t)()

	var ss SpanSet

random idea: call this bdGkq to make it self-describing and to make these tests easier to read (this test is small anyway, but maybe this works in general).

---

pkg/storage/spanset/spanset_test.go, line 235 at r1 (raw file):

	}
	for _, span := range allowed {
		if err := ss.checkAllowed(SpanReadOnly, span, true /* spanKeyExclusive */); err != nil {

Ok this explains why I'm confused. spanKeyExclusive makes the end key inclusive? I see how that's the right thing for the SeekLT usage but the naming is confusing.

[petermattis]

Glad to have you in this area of the code! The changes look very good. I'm not too worried that things will have slowed down due to the reverse iteration, but others will have more informed opinions on that.

Reverse iteration is definitely slower than forward iteration (25-50% depending on what/where you're measuring). But this can be well worth it if we are scanning less data due to using reverse iteration. Is that the case here? I haven't grok'd what is going on in this PR.

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

[tbg]

No, I think we're scanning the exact same data and it's basically "all of the data". Where previously we'd iterate through all of the kv pairs in a range in forward order, we're now doing the reverse. There is an opportunity to "optimize" by seeking to the previous user key the moment current one's versions are no longer GC'able. Seek internally steps the iter a few times before resorting to the actual seek (as you well know). In this PR as written, we will manually step the iterator through every kv pair (the old code did that too, but not in reverse). Whenever we have lots of keys with >>10 non-gcable versions the actual Seek should be the better choice (and should be equivalent otherwise, due to the internal stepping optimization).
My intuition is that the slowdown of reverse iteration will not be felt much because GC is not a foreground operation and because it hasn't been performance optimized, so there will be other bottlenecks. But it seems like we should check.

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

[ajwerner]

Thanks for giving it a look. I’ll move forward with writing benchmarks and testing.

[ajwerner]

Alright, I wrote some benchmarks and some tests. I also took this chance to carve off a new package.

The test coverage is no worse than it used to be because I left all of the old tests in place. I also wrote a test that generates a random dataset and compares the output of the old implementation to the new one. Perhaps a few more unit tests are in order and perhaps more coverage of the unchanged code related to abort spans and the local key range.

After some minor memory optimizations the benchmarks show a modest win and a dramatic reduction in memory usage.

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

---

pkg/storage/batch_spanset_test.go, line 193 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Lower bound, right? Span set has [c,g), and insideKey==c.

Right. Fixed.

---

pkg/storage/gc_iterator.go, line 40 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This is dangerously close to warranting a struct that is being returned. You're often returning mostly zero values, so it would actually help readability quite a bit if you introduced

type gcIterState struct {
  cur, next *engine.MVCCKeyValue
  isNewest, isIntent, ok bool
  err error

It would also give you a more convenient place for comments.
Is ok really needed? A comment explaining what it does would help.

I'll assume this method does the right thing for now, will revisit in post your next round of changes.

I did indeed create a struct. I also simplified the logic.

---

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

Previously, tbg (Tobias Grieger) wrote…

Can you add a comment for spanKeyExclusive? Something like

If spanKeyExclusive is set, the spans in access will be made open, i.e. [a,b) will be considered (a,b).

spans in access will be open at the start and closed at the end, i.e. [a,b) will be considered (a,b]

---

pkg/storage/spanset/spanset.go, line 195 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Isn't spanKeyExclusive always true if this expr is evaluated?

Having read the tests I think what's happening here is that cur = [k1, k2) is treated like (k1, k2]. I find the bool is not named intuitively for that behavior. Could "reverse" (along with a comment) be a better name?

I've renamed the variable and added commentary.

---

pkg/storage/spanset/spanset_test.go, line 224 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

random idea: call this bdGkq to make it self-describing and to make these tests easier to read (this test is small anyway, but maybe this works in general).

Done.

---

pkg/storage/spanset/spanset_test.go, line 235 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Ok this explains why I'm confused. spanKeyExclusive makes the end key inclusive? I see how that's the right thing for the SeekLT usage but the naming is confusing.

Hopefully it's less confusing now.

---

pkg/storage/engine/gc.go, line 36 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Write 'next' here to make this easier to parse.

Done.

---

pkg/storage/engine/gc.go, line 37 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

non-nil,

s/next/newer/

Done.

---

pkg/storage/engine/gc.go, line 40 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Explain that there being an intent doesn't matter in the impl below. Conservatively we have to assume that the intent will get aborted, so we will be able to GC just the values that we could remove if there weren't an intent. Maybe this comment doesn't have to live in the method comment, but it would be helpful somewhere.

Done.

---

pkg/storage/engine/gc_test.go, line 13 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Do these tests need to be replaced?

definitely. I retained them in the testing file which now contains all of this old code and exists to support the old implementation.

[ajwerner]

@tbg friendly ping

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

[tbg]

Looks great! and thanks for all of the cleanup work. I might've missed some of the changes since my last review (Reviewable and/or I got a little confused with all the revisions), so if there's something in particular that I should give a closer look please let me know.

Reviewed 13 of 13 files at r7, 7 of 7 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 7 of 7 files at r11, 12 of 12 files at r12.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/storage/gc/gc_iterator_test.go, line 27 at r12 (raw file):

)

func TestGCIterator(t *testing.T) {

Bit of commentary as this test proceeds would be nice.

---

pkg/storage/gc/gc_old_test.go, line 32 at r12 (raw file):

)

// RunGCOld is an older implementation of Run. It is used for benchmarking and

Not that it matters much, but does it need to be exported?

---

pkg/storage/gc/gc.go, line 477 at r12 (raw file):

// multiple of the heartbeat timeout used by the coordinator.
//
// TODO(tschottdorf): this could be done in Replica.GC itself, but it's

This TODO can be removed (not worth a CI run though)

[tbg]

(and sorry for letting this sit - I've changed my GH workflow a bit and am still working out the kinks).

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

[ajwerner]

TFTR!

bors r=tbg

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

---

pkg/storage/gc/gc.go, line 477 at r12 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This TODO can be removed (not worth a CI run though)

Done.

---

pkg/storage/gc/gc_iterator_test.go, line 27 at r12 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Bit of commentary as this test proceeds would be nice.

Done.

---

pkg/storage/gc/gc_old_test.go, line 32 at r12 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Not that it matters much, but does it need to be exported?

When I first wrote it I made the benchmarks a package gc_test file, I ended up reverting that because symbol sharing with other testing infrastructure got annoying. Unexported.

[craig]

Build succeeded

• GitHub CI (Cockroach)