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

internal/daemon/compact.go

@@ -19,6 +19,9 @@ const (
 	compactEvery = 3 * time.Hour
 	// compactPace is a pause between snapshots, as for the daily check.
 	compactPace = 50 * time.Millisecond
+	// dbCompactFirst is when the database file first gives its unused space
+	// back (see daemon.go), once the start's own work is done.
+	dbCompactFirst = 2 * time.Minute
 )
 
 // CompactSnapshots has every snapshot that can share its large files with

internal/daemon/daemon.go

@@ -386,6 +386,37 @@ func (d *Daemon) Start() error {
 		}
 	})
 
+	// The database file gives back the space of what was deleted from it
+	// (store/compact.go): shortly after start, out of the way of everything a
+	// start does, and hourly after that. A database from an older build is
+	// switched over first, once: a rewrite of the whole file, which in
+	// store.Open would have held up the first start after updating.
+	d.P2P.GoSync(func(ctx context.Context) {
+		select {
+		case <-ctx.Done():
+			return
+		case <-time.After(dbCompactFirst):
+		}
+		began := time.Now()
+		if converted, err := d.Store.EnableIncrementalVacuum(); err != nil {
+			d.Log.Log("warn", fmt.Sprintf("the database could not be set to give unused space back to the disk: %v", err))
+		} else if converted {
+			d.Log.Log("info", fmt.Sprintf("the database now gives unused space back to the disk (a one-time rewrite, %s)",
+				time.Since(began).Round(100*time.Millisecond)))
+		}
+		d.Store.Compact()
+		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

@@ -416,8 +416,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

@@ -1243,14 +1243,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,73 @@
+package store
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+
+	"github.com/opensave/opensave/internal/fsx"
+)
+
+// 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. A database
+// being created gets the setting for free: it takes effect only on an empty
+// file, so openDB asks for it before anything else. One created by an older
+// build needs a full VACUUM to take it (EnableIncrementalVacuum), which the
+// daemon runs once in the background after it has started, not in Open: on a
+// large database that is a rewrite of some seconds, and the first start after
+// updating would wait for it.
+
+// 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) incrementalVacuum() bool {
+	var mode int
+	return s.db.Get(&mode, `PRAGMA auto_vacuum`) == nil && mode == 2 // 2 = INCREMENTAL
+}
+
+// EnableIncrementalVacuum switches a database created by an older build to
+// incremental auto-vacuum, with the one full VACUUM that takes. It reports
+// whether it did; on a database that already has it, it does nothing.
+//
+// The VACUUM rewrites the whole file: it holds the database while it runs and
+// needs about the file's size free on the same disk for the copy. Without
+// that much free it is not attempted, and the error says so.
+func (s *Store) EnableIncrementalVacuum() (bool, error) {
+	if s.incrementalVacuum() {
+		return false, nil
+	}
+	if s.path != "" {
+		if info, err := os.Stat(s.path); err == nil {
+			if free, ok := fsx.FreeBytes(filepath.Dir(s.path)); ok && free < uint64(info.Size()) {
+				return false, fmt.Errorf("giving unused space back needs about %d MB free next to the database, and %d MB are",
+					info.Size()>>20, free>>20)
+			}
+		}
+	}
+	if _, err := s.db.Exec(`PRAGMA auto_vacuum = INCREMENTAL`); err != nil {
+		return false, err
+	}
+	if _, err := s.db.Exec(`VACUUM`); err != nil {
+		return false, err
+	}
+	return true, nil
+}
+
+// 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,137 @@
+package store
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/jmoiron/sqlx"
+)
+
+// 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)
+	}
+}
+
+// A database created by an older build, without incremental auto-vacuum, is
+// not rewritten when it is opened: the start stays fast. It is switched over
+// once, by EnableIncrementalVacuum, and after that gives space back as usual.
+func TestOlderDatabaseIsSwitchedOverAfterOpenNotDuringIt(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "opensave.db")
+	old, err := sqlx.Open("sqlite", "file:"+path) // as an older build left it: no auto-vacuum
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := old.Exec(`CREATE TABLE older_build (k TEXT)`); err != nil {
+		t.Fatal(err)
+	}
+	old.Close()
+
+	s, err := Open(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+	if s.incrementalVacuum() {
+		t.Fatal("Open rewrote an older database; that is for after the start")
+	}
+	converted, err := s.EnableIncrementalVacuum()
+	if err != nil || !converted {
+		t.Fatalf("EnableIncrementalVacuum = %v, %v; want it switched over", converted, err)
+	}
+	if !s.incrementalVacuum() {
+		t.Fatal("auto_vacuum is still not incremental")
+	}
+	if converted, err := s.EnableIncrementalVacuum(); err != nil || converted {
+		t.Errorf("a second EnableIncrementalVacuum = %v, %v; want nothing to do", converted, err)
+	}
+}

internal/store/db.go

@@ -23,7 +23,8 @@ var migrationsFS embed.FS
 // methods (see settings.go, games.go, branches.go, snapshots.go, peers.go,
 // cloudtokens.go).
 type Store struct {
-	db *sqlx.DB
+	db   *sqlx.DB
+	path string // the database file, for EnableIncrementalVacuum's space check
 }
 
 // Open creates (if needed) and opens the SQLite database at path, applying
@@ -34,7 +35,7 @@ func Open(path string) (*Store, error) {
 		return nil, err
 	}
 
-	s := &Store{db: db}
+	s := &Store{db: db, path: path}
 	if err := s.migrate(); err != nil {
 		db.Close()
 		return nil, err
@@ -55,7 +56,18 @@ func openDB(path string) (*sqlx.DB, 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.
+	//
+	// auto_vacuum INCREMENTAL comes first: it takes effect only while the
+	// file is still empty, before WAL writes its header (see compact.go). On
+	// an existing database it changes nothing.
+	dsn := fmt.Sprintf("file:%s?_pragma=auto_vacuum(2)&_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)

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.