Database: WAL, batched clears of deletion records, and giving free space back

internal/daemon/daemon.go

@@ -366,6 +366,21 @@ func (d *Daemon) Start() error {
 		}
 	})
 
+	// The database file gives back the space of what was deleted from it
+	// (store/compact.go), hourly.
+	d.P2P.GoSync(func(ctx context.Context) {
+		ticker := time.NewTicker(time.Hour)
+		defer ticker.Stop()
+		for {
+			select {
+			case <-ctx.Done():
+				return
+			case <-ticker.C:
+				d.Store.Compact()
+			}
+		}
+	})
+
 	// Look for newly installed games: a few minutes after start, then every
 	// hour. See newgames.go.
 	d.P2P.GoSync(func(ctx context.Context) {

internal/p2p/syncengine/hold.go

@@ -382,8 +382,8 @@ func (e *Engine) PutBack(gameID string) (fetching int, err error) {
 	}
 	for root, files := range hold.Files() {
 		var missing []string
+		_ = e.Store.ClearDeletedFiles(gameID, root, files)
 		for _, f := range files {
-			_ = e.Store.ClearDeletedFile(gameID, root, f)
 			if _, ok := x.present[root][f]; !ok {
 				missing = append(missing, f)
 			}

internal/snapshot/snapshot.go

@@ -1195,14 +1195,16 @@ func (m *Manager) recordDeletionsSince(gameID, branch, snapshotID string, captur
 		if err != nil || len(records) == 0 {
 			continue
 		}
+		var back []string
 		for _, f := range captured {
 			if f.Root != root {
 				continue
 			}
 			if _, remembered := records[f.Path]; remembered {
-				_ = m.Store.ClearDeletedFile(gameID, root, f.Path)
+				back = append(back, f.Path)
 			}
 		}
+		_ = m.Store.ClearDeletedFiles(gameID, root, back)
 	}
 }
 

internal/store/compact.go

@@ -0,0 +1,43 @@
+package store
+
+// Giving the space of deleted rows back to the disk.
+//
+// SQLite keeps the pages rows were deleted from, for rows to come; the file
+// never shrinks on its own. Snapshots are merged, pruned and replaced all the
+// time, and each held a row per file of its save — a quarter of a million
+// for one game — so the file grew by what every snapshot ever deleted had
+// held: 663 MB on one device, 262 MB of it unused.
+//
+// With auto_vacuum set to INCREMENTAL the free pages can be handed back a
+// few at a time (Compact), never holding the database for long. Setting it
+// takes one full VACUUM, done once, when the database is opened and nothing
+// else is using it: some seconds for a large one.
+
+// freePagesWorthCompacting is how many free pages are given back at a time,
+// and fewer are not worth the bother: 4 MB at the default page size.
+const freePagesWorthCompacting = 1024
+
+func (s *Store) compactOnOpen() {
+	var mode int
+	if err := s.db.Get(&mode, `PRAGMA auto_vacuum`); err != nil {
+		return
+	}
+	if mode != 2 { // 2 = INCREMENTAL
+		if _, err := s.db.Exec(`PRAGMA auto_vacuum = INCREMENTAL`); err != nil {
+			return
+		}
+		_, _ = s.db.Exec(`VACUUM`)
+		return
+	}
+	s.Compact()
+}
+
+// Compact hands the database file's unused pages back to the disk, when
+// there are enough of them to matter.
+func (s *Store) Compact() {
+	var free int
+	if err := s.db.Get(&free, `PRAGMA freelist_count`); err != nil || free < freePagesWorthCompacting {
+		return
+	}
+	_, _ = s.db.Exec(`PRAGMA incremental_vacuum`)
+}

internal/store/compact_test.go

@@ -0,0 +1,101 @@
+package store
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+)
+
+// Deleted rows give their space back to the disk: the file shrinks, rather
+// than holding everything it ever held.
+func TestCompactGivesSpaceBack(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "opensave.db")
+	s, err := Open(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var mode int
+	_ = s.db.Get(&mode, `PRAGMA auto_vacuum`)
+	if mode != 2 {
+		t.Fatalf("auto_vacuum = %d, want incremental (2)", mode)
+	}
+	// A table of the test's own, so the test depends on no other.
+	if _, err := s.db.Exec(`CREATE TABLE padding (k TEXT PRIMARY KEY, v TEXT NOT NULL)`); err != nil {
+		t.Fatal(err)
+	}
+	pad := strings.Repeat("x", 2000)
+	tx := s.db.MustBegin()
+	for i := 0; i < 4000; i++ {
+		tx.MustExec(`INSERT INTO padding (k, v) VALUES (?, ?)`, fmt.Sprint("k", i), pad)
+	}
+	if err := tx.Commit(); err != nil {
+		t.Fatal(err)
+	}
+	_, _ = s.db.Exec(`PRAGMA wal_checkpoint(TRUNCATE)`)
+	full, _ := os.Stat(path)
+	if _, err := s.db.Exec(`DELETE FROM padding`); err != nil {
+		t.Fatal(err)
+	}
+	s.Compact()
+	_, _ = s.db.Exec(`PRAGMA wal_checkpoint(TRUNCATE)`)
+	after, _ := os.Stat(path)
+	if after.Size() >= full.Size()/2 {
+		t.Errorf("file %d bytes after deleting everything, %d before: nothing given back", after.Size(), full.Size())
+	}
+	s.Close()
+}
+
+// Clearing a large save's deletion records is one transaction, not one write
+// to disk per file.
+func TestClearDeletedFilesIsOneTransaction(t *testing.T) {
+	s, err := Open(filepath.Join(t.TempDir(), "opensave.db"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+	if err := s.CreateGame(Game{ID: "g", Name: "g", SavePath: t.TempDir(), ActiveBranch: "main"}); err != nil {
+		t.Fatal(err)
+	}
+	var files []DeletedFile
+	var paths []string
+	for i := 0; i < 50000; i++ {
+		p := fmt.Sprintf("map/%d/%d.bin", i/100, i)
+		files = append(files, DeletedFile{Path: p, Hash: "h"})
+		paths = append(paths, p)
+	}
+	if err := s.RecordDeletedFiles("g", files); err != nil {
+		t.Fatal(err)
+	}
+	start := time.Now()
+	if err := s.ClearDeletedFiles("g", "", paths); err != nil {
+		t.Fatal(err)
+	}
+	took := time.Since(start)
+	left, _ := s.DeletedFiles("g", "")
+	if len(left) != 0 {
+		t.Errorf("%d records left", len(left))
+	}
+	// Generous: CI runs this under the race detector on shared runners, where
+	// one statement per file took 10s; what this guards against took minutes.
+	if took > 30*time.Second {
+		t.Errorf("clearing 50,000 records took %v", took)
+	}
+	t.Logf("cleared 50,000 deletion records in %v", took)
+}
+
+// The database writes through a write-ahead log, not a rollback journal
+// created and deleted for every write.
+func TestDatabaseUsesWAL(t *testing.T) {
+	s, err := Open(filepath.Join(t.TempDir(), "opensave.db"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+	var mode string
+	if err := s.db.Get(&mode, `PRAGMA journal_mode`); err != nil || mode != "wal" {
+		t.Errorf("journal_mode = %q (%v), want wal", mode, err)
+	}
+}

internal/store/db.go

@@ -32,7 +32,14 @@ func Open(path string) (*Store, error) {
 	// _pragma params ensure foreign keys are enforced (SQLite defaults them
 	// off per-connection) and busy_timeout avoids spurious SQLITE_BUSY
 	// errors from the watcher/api/p2p goroutines all touching the DB.
-	dsn := fmt.Sprintf("file:%s?_pragma=foreign_keys(1)&_pragma=busy_timeout(5000)", path)
+	//
+	// WAL: a write is appended to one log and the database file is updated
+	// from it later, rather than every write creating, flushing and deleting a
+	// rollback journal of its own — which made a quarter of a million small
+	// writes take most of an hour. synchronous NORMAL is what WAL is meant to
+	// run with: the database stays whole whatever happens; a power cut can
+	// only lose the last moments' writes, never corrupt it.
+	dsn := fmt.Sprintf("file:%s?_pragma=foreign_keys(1)&_pragma=busy_timeout(5000)&_pragma=journal_mode(WAL)&_pragma=synchronous(NORMAL)", path)
 	db, err := sqlx.Open("sqlite", dsn)
 	if err != nil {
 		return nil, fmt.Errorf("open sqlite database: %w", err)
@@ -51,6 +58,7 @@ func Open(path string) (*Store, error) {
 		db.Close()
 		return nil, err
 	}
+	s.compactOnOpen()
 	// The file holds the device's private key, the Google tokens and the
 	// vault keys. SQLite creates it with the process umask, which is usually
 	// world-readable; the directory around it is private now, but the file

internal/store/deleted_files.go

@@ -107,6 +107,41 @@ func (s *Store) ClearDeletedFile(gameID, root, path string) error {
 	return nil
 }
 
+// ClearDeletedFiles forgets the deletions recorded for paths of one save
+// location, in one transaction: one at a time, each its own write to disk,
+// the quarter of a million of a large save took a quarter of an hour. Paths go
+// clearDeletedChunk at a time, so that is a few hundred statements, not one
+// per file.
+func (s *Store) ClearDeletedFiles(gameID, root string, paths []string) error {
+	if len(paths) == 0 {
+		return nil
+	}
+	tx, err := s.db.Beginx()
+	if err != nil {
+		return err
+	}
+	defer tx.Rollback()
+	for len(paths) > 0 {
+		n := min(len(paths), clearDeletedChunk)
+		args := make([]any, 0, n+2)
+		args = append(args, gameID, root)
+		for _, p := range paths[:n] {
+			args = append(args, p)
+		}
+		q := `DELETE FROM deleted_files WHERE game_id = ? AND root = ? AND path IN (?` +
+			strings.Repeat(",?", n-1) + `)`
+		if _, err := tx.Exec(q, args...); err != nil {
+			return fmt.Errorf("clear deletion records: %w", err)
+		}
+		paths = paths[n:]
+	}
+	return tx.Commit()
+}
+
+// clearDeletedChunk is how many paths one statement of ClearDeletedFiles
+// names: well under SQLite's limit on bound parameters.
+const clearDeletedChunk = 500
+
 // ClearDeletedFilesForGame forgets every deletion recorded for a game, for use
 // when it is untracked — the records describe a folder this device no longer
 // has an opinion about.