dolt gc --full: Implement a flag for GC which will collect everything, including the old gen. #8462

reltuk · 2024-10-17T10:12:36Z

Dolt GC is generational. By default, it moves all chunks reachable from commits into the old gen, and it leaves uncommitted data in the new gen. By default, it never revisits the chunks in the old gen. This means that if a branch with a lot of unique data exists during a GC, and then gets deleted, the storage taken up by that branch is never reclaimed by future garbage collection.

This PR implements dolt gc --full, which performs a collection on the entire database, ignoring the generational aspects of previous collections and visiting the entire set of live data. At the end of a successful dolt gc --full, only data which was reachable at the start of the run and data which has been written during the run will be present in the database.

Note: dolt gc --full visits every reachable chunk in the database and can be resource intensive. As a consequence of running dolt gc --full, any previously archived table files will be unarchived, and storage savings from the archiving will be lost. dolt gc --full involves copying chunks into new storage files, and fully materializing the new files before the old files are deleted; as a consequence, peak disk utilisation while running dolt gc --full can be up to 2x the existing size of the database.

…mode GC through to doltdb.DDB.GC call.

…not a *NomsBlockStore.

…ns a finalizer. Previously MarkAndSweepChunks would finalize itself by either appending the table table files to the store or by swaping to the table files. Now it returns a GCFinalizer instance which is used by the caller to effect the desired change. This is necessary for implementing Full GC, where we actually want to do both with the same GC result, but at different phases of the GC process. This commit leaves the HasManyF return from NomsBlockStore.MarkAndSweepChunks as a TODO.

…added to the store as part of the GC can be checked for their contents.

This does not yet work because the source ChunkStore is currently newGen. Still reworking so that the generational store itself is the source.

Change ValueStore's handling of generational chunk stores, so that it sweeps chunks from the generational store itself, and into new gen and old gen. This will support the full gc access pattern where we need to copy chunks from old gen into a new table file which will appear in old gen itself.

… that dolt gc --full removes data from old gen which is now unreachable.

coffeegoddd · 2024-10-17T10:42:43Z

@reltuk DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`88a9a34`	ok	5937457

version	total_tests
`88a9a34`	5937457

correctness_percentage
100.0

…ves will still break `dolt gc --full`.

coffeegoddd · 2024-10-17T11:29:05Z

@reltuk DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`077294f`	ok	5937457

version	total_tests
`077294f`	5937457

correctness_percentage
100.0

…sed by just fetching the chunks and re-compressing them. This makes `dolt gc` work while sourcing the chunks from the generational store and makes `dolt gc --full` work against databases which have been previously archived. `dolt gc --full` will still completely undo the archiving in the process of running the garbage collection.

coffeegoddd · 2024-10-17T16:27:47Z

@reltuk DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`6cfac1b`	ok	5937457

version	total_tests
`6cfac1b`	5937457

correctness_percentage
100.0

macneale4

Nit comments and questions. No show stoppers.

macneale4 · 2024-10-17T16:29:33Z

go/store/chunks/chunk_store.go

@@ -152,6 +152,32 @@ type LoggingChunkStore interface {

 var ErrAddChunkMustBlock = errors.New("chunk keeper: add chunk must block")

+type HasManyF func(ctx context.Context, hashes hash.HashSet) (absent hash.HashSet, err error)


HasManyFunc would better, IMO. It's just 24bits man. come on.

Also, gDoc. Dangling function defs like this end up being abused if you don't.

macneale4 · 2024-10-17T16:47:11Z

go/store/types/value_store.go

 		if err != nil {
 			return err
 		}

 		root, err := lvs.Root(ctx)
 		if err != nil {
-			newGen.EndGC()
+			collector.EndGC()


Is there a reason you aren't simplifying this and doing a defer collector.EndGC() in both places where you BeginGC() ?

Looks like we always want to EndGC() so it would be more readable if you did

It was just a lot of nesting...but in converting it I found a place where we forgot the EndGC. So...this was definitely good feedback. Thanks for it.

macneale4 · 2024-10-17T16:59:49Z

go/libraries/doltcore/doltdb/gc_test.go

@@ -139,7 +139,7 @@ func testGarbageCollection(t *testing.T, test gcTest) {
 		}
 	}

-	err := dEnv.DoltDB.GC(ctx, nil)
+	err := dEnv.DoltDB.GC(ctx, types.GCModeDefault, nil)


These tests are weird. Just an observation. I was thinking that I was going to ask for a test for the Full flag here, but it looks like these tests are basically just running Exec. Maybe bats is better than this?

sqlengine tests for this instead? Just sayin. These tests are weird.

Yeah, I don't think these tests are worth much compared to a bats test which does the same. Obviously they're supposed to target the DoltDB.GC invocation itself, and they're much easy to attach a debugger to or whatever, but AFAICT they're not providing much in the way of protection compared to existing layers of cheese.

macneale4 · 2024-10-17T17:10:05Z

go/store/chunks/memory_store.go

@@ -343,7 +343,24 @@ func (ms *MemoryStoreView) EndGC() {
 	ms.transitionToNoGC()
 }

-func (ms *MemoryStoreView) MarkAndSweepChunks(ctx context.Context, hashes <-chan []hash.Hash, dest ChunkStore) error {
+type msvGcFinalizer struct {


After GC, what does the memory store contain? This refactor looks like it preserves previous behavior, but I'm not sure why the previous behavior is the way it is. Educate me

AFAIK, memory store is an in-memory chunk store implementation. After GC, memory store just contains all the visited chunks – that is, all the chunks whose address came through on the keepers channel.

go/store/types/value_store.go

macneale4 · 2024-10-17T17:30:44Z

go/store/nbs/store.go

@@ -1037,6 +1037,34 @@ func (nbs *NomsBlockStore) HasMany(ctx context.Context, hashes hash.HashSet) (ha
 	return nbs.hasMany(toHasRecords(hashes))
 }

+func (nbs *NomsBlockStore) hasManyInSources(ctx context.Context, srcs []hash.Hash, hashes hash.HashSet) (hash.HashSet, error) {


Is the ctx future proofing for when we might need it? No used currently. If you think the interface needs it, I'd drop the name and make it '_' so people know that this is intentional

I just didn't realize it was unneeded. Removed! Thanks for that :).

go/store/nbs/table_set.go

macneale4

                __/___            
          _____/______|           
  _______/_____\_______\_____     
  \              < < <       |    
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

coffeegoddd · 2024-10-17T19:18:09Z

@reltuk DOLT

comparing_percentages
100.000000 to 100.000000

version	result	total
`0a53805`	ok	5937457

version	total_tests
`0a53805`	5937457

correctness_percentage
100.0

reltuk added 9 commits October 15, 2024 02:57

go/cmd/dolt: commands/gc: Add flag handling for --full flag.

fc12a80

go/libraries/doltcore/{doltdb,sqle/dprocedures}: Thread full/default …

c4e485f

…mode GC through to doltdb.DDB.GC call.

go/store/nbs: store.go: Add error checking for case where destNBS is …

cc47d3f

…not a *NomsBlockStore.

go/store/nbs: Implement GCFinalizer HasManyF so that the table files …

e37acff

…added to the store as part of the GC can be checked for their contents.

go/store/types: Implement logic for a full GC.

95e2adb

This does not yet work because the source ChunkStore is currently newGen. Still reworking so that the generational store itself is the source.

integration-tests/bats: garbage_collection.bats: Add a test asserting…

0905d48

… that dolt gc --full removes data from old gen which is now unreachable.

go/store/{nbs,types}: Small cleanups for gc --full.

88a9a34

coffeegoddd added the correctness_approved label Oct 17, 2024

go/store/types: Fix dolt gc for the archive use case, even if archi…

077294f

…ves will still break `dolt gc --full`.

macneale4 self-requested a review October 17, 2024 15:49

macneale4 approved these changes Oct 17, 2024

View reviewed changes

go/store/{nbs,chunks,types}: PR feedback.

0a53805

macneale4 approved these changes Oct 17, 2024

View reviewed changes

reltuk merged commit d58a69a into main Oct 17, 2024
21 checks passed

BrewTestBot mentioned this pull request Oct 17, 2024

dolt 1.43.7 Homebrew/homebrew-core#194815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dolt gc --full: Implement a flag for GC which will collect everything, including the old gen. #8462

dolt gc --full: Implement a flag for GC which will collect everything, including the old gen. #8462

reltuk commented Oct 17, 2024 •

edited

Loading

coffeegoddd commented Oct 17, 2024

coffeegoddd commented Oct 17, 2024

coffeegoddd commented Oct 17, 2024

macneale4 left a comment

macneale4 Oct 17, 2024

macneale4 Oct 17, 2024

reltuk Oct 17, 2024

macneale4 Oct 17, 2024

reltuk Oct 17, 2024

macneale4 Oct 17, 2024

reltuk Oct 17, 2024

macneale4 Oct 17, 2024

reltuk Oct 17, 2024

macneale4 left a comment

coffeegoddd commented Oct 17, 2024

		@@ -152,6 +152,32 @@ type LoggingChunkStore interface {

		var ErrAddChunkMustBlock = errors.New("chunk keeper: add chunk must block")

		type HasManyF func(ctx context.Context, hashes hash.HashSet) (absent hash.HashSet, err error)

dolt gc --full: Implement a flag for GC which will collect everything, including the old gen. #8462

dolt gc --full: Implement a flag for GC which will collect everything, including the old gen. #8462

Conversation

reltuk commented Oct 17, 2024 • edited Loading

coffeegoddd commented Oct 17, 2024

coffeegoddd commented Oct 17, 2024

coffeegoddd commented Oct 17, 2024

macneale4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macneale4 left a comment

Choose a reason for hiding this comment

coffeegoddd commented Oct 17, 2024

reltuk commented Oct 17, 2024 •

edited

Loading