From cdf7cbdea1e52645455e14a45310f66373e764de Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Sun, 10 Dec 2023 10:30:52 -0500 Subject: [PATCH] Handle snapshots for empty WALs --- db/db.go | 6 ++- db/wal/compacting_scanner_test.go | 7 +++ snapshot/sink.go | 63 ++++++++++++++++-------- snapshot/sink_test.go | 5 ++ snapshot/store.go | 1 - snapshot/testdata/db-and-wals/empty-file | 0 6 files changed, 59 insertions(+), 23 deletions(-) create mode 100644 snapshot/testdata/db-and-wals/empty-file diff --git a/db/db.go b/db/db.go index 527f70b6..d33c0d46 100644 --- a/db/db.go +++ b/db/db.go @@ -106,6 +106,7 @@ type PoolStats struct { } // IsValidSQLiteFile checks that the supplied path looks like a SQLite file. +// A non-existent file is considered invalid. func IsValidSQLiteFile(path string) bool { f, err := os.Open(path) if err != nil { @@ -122,13 +123,14 @@ func IsValidSQLiteFile(path string) bool { } // IsValidSQLiteData checks that the supplied data looks like a SQLite data. -// See https://www.sqlite.org/fileformat.html +// See https://www.sqlite.org/fileformat.html. func IsValidSQLiteData(b []byte) bool { return len(b) > 13 && string(b[0:13]) == "SQLite format" } // IsValidSQLiteWALFile checks that the supplied path looks like a SQLite -// WAL file. See https://www.sqlite.org/fileformat2.html#walformat +// WAL file. See https://www.sqlite.org/fileformat2.html#walformat. A +// non-existent file is considered invalid. func IsValidSQLiteWALFile(path string) bool { f, err := os.Open(path) if err != nil { diff --git a/db/wal/compacting_scanner_test.go b/db/wal/compacting_scanner_test.go index dc091104..77b65188 100644 --- a/db/wal/compacting_scanner_test.go +++ b/db/wal/compacting_scanner_test.go @@ -7,6 +7,13 @@ import ( "testing" ) +func Test_CompactingScanner_Scan_Empty(t *testing.T) { + _, err := NewCompactingScanner(bytes.NewReader([]byte{}), true) + if err != io.EOF { + t.Fatal(err) + } +} + func Test_CompactingScanner_Scan(t *testing.T) { b, err := os.ReadFile("testdata/wal-reader/ok/wal") if err != nil { diff --git a/snapshot/sink.go b/snapshot/sink.go index 7281fe58..863152ee 100644 --- a/snapshot/sink.go +++ b/snapshot/sink.go @@ -77,7 +77,9 @@ func (s *Sink) Cancel() error { } // Close closes the sink, and finalizes creation of the snapshot. It is critical -// that Close is called, or the snapshot will not be in place. +// that Close is called, or the snapshot will not be in place. It is OK to call +// Close without every calling Write. In that case the Snapshot will be finalized +// as usual, but will effectively be the same as the previously created snapshot. func (s *Sink) Close() error { if !s.opened { return nil @@ -132,28 +134,38 @@ func (s *Sink) processSnapshotData() (retErr error) { return err } - if db.IsValidSQLiteFile(s.dataFD.Name()) { - if err := os.Rename(s.dataFD.Name(), filepath.Join(s.str.Dir(), s.meta.ID+".db")); err != nil { - return err - } - } else if db.IsValidSQLiteWALFile(s.dataFD.Name()) { - if len(snapshots) == 0 { - // We are trying to create our first snapshot from a WAL file, which is invalid. - return fmt.Errorf("data for first snapshot is a WAL file") - } else { - // We have at least one previous snapshot. That means we should have a valid SQLite file - // for the previous snapshot. + if len(snapshots) == 0 && !db.IsValidSQLiteFile(s.dataFD.Name()) { + // We have no snapshots yet, so the incomding data must be a valid SQLite file. + return fmt.Errorf("data for first snapshot must be a valid SQLite file") + } + + dataSz, err := fileSize(s.dataFD.Name()) + if err != nil { + return err + } + + // Writing zero data for a snapshot is acceptable, and indicates the snapshot + // is empty. This could happen if lots of entries were written to the Raft log, + // which would trigger a Raft snapshot, but those entries didn't actually change + // the database. Otherwise, the data must be a valid SQLite file or WAL file. + if dataSz != 0 { + if db.IsValidSQLiteFile(s.dataFD.Name()) { + if err := os.Rename(s.dataFD.Name(), filepath.Join(s.str.Dir(), s.meta.ID+".db")); err != nil { + return err + } + } else if db.IsValidSQLiteWALFile(s.dataFD.Name()) { + // With WAL data incoming, then we must have a valid SQLite file from the previous snapshot. snapPrev := snapshots[len(snapshots)-1] snapPrevDB := filepath.Join(s.str.Dir(), snapPrev.ID+".db") if !db.IsValidSQLiteFile(snapPrevDB) { return fmt.Errorf("previous snapshot data is not a SQLite file: %s", snapPrevDB) } + if err := os.Rename(s.dataFD.Name(), filepath.Join(s.str.Dir(), s.meta.ID+".db-wal")); err != nil { + return err + } + } else { + return fmt.Errorf("invalid snapshot data file: %s", s.dataFD.Name()) } - if err := os.Rename(s.dataFD.Name(), filepath.Join(s.str.Dir(), s.meta.ID+".db-wal")); err != nil { - return err - } - } else { - return fmt.Errorf("invalid snapshot data file: %s", s.dataFD.Name()) } // Indicate snapshot data been successfully persisted to disk by renaming @@ -178,12 +190,15 @@ func (s *Sink) processSnapshotData() (retErr error) { snapNewDB := filepath.Join(s.str.Dir(), snapNew.ID+".db") snapNewWAL := filepath.Join(s.str.Dir(), snapNew.ID+".db-wal") - if db.IsValidSQLiteWALFile(snapNewWAL) { - // The most recent snapshot was created from a WAL file, so we need to replay - // that WAL file into the previous SQLite file. + if db.IsValidSQLiteWALFile(snapNewWAL) || dataSz == 0 { + // One of two things have happened. Either the snapshot data is empty, in which + // case we can just make the existing SQLite file the new snapshot, or the snapshot + // data is a valid WAL file, in which case we need to replay it into the existing + // SQLite file. if err := os.Rename(snapPrevDB, snapNewDB); err != nil { return err } + // An open-close cycle checkpoints and removes any WAL file. if err := openCloseDB(snapNewDB); err != nil { return err } @@ -200,3 +215,11 @@ func (s *Sink) processSnapshotData() (retErr error) { func (s *Sink) writeMeta(dir string) error { return writeMeta(dir, s.meta) } + +func fileSize(path string) (int64, error) { + stat, err := os.Stat(path) + if err != nil { + return 0, err + } + return stat.Size(), nil +} diff --git a/snapshot/sink_test.go b/snapshot/sink_test.go index db329f59..f53b7123 100644 --- a/snapshot/sink_test.go +++ b/snapshot/sink_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "path/filepath" "testing" "github.com/hashicorp/raft" @@ -269,12 +270,16 @@ func Test_SinkCreateFullThenWALSnapshots(t *testing.T) { createSnapshot("snap-3456", 5, 4, 3, "testdata/db-and-wals/wal-01") createSnapshot("snap-4567", 6, 5, 4, "testdata/db-and-wals/wal-02") createSnapshot("snap-5678", 7, 6, 5, "testdata/db-and-wals/wal-03") + createSnapshot("snap-9abc", 8, 7, 6, "testdata/db-and-wals/empty-file") // Check the database state inside the Store. dbPath, err := store.getDBPath() if err != nil { t.Fatalf("Failed to get DB path: %v", err) } + if filepath.Base(dbPath) != "snap-9abc.db" { + t.Fatalf("Unexpected DB file name: %s", dbPath) + } checkDB, err := db.Open(dbPath, false, true) if err != nil { t.Fatalf("failed to open database at %s: %s", dbPath, err) diff --git a/snapshot/store.go b/snapshot/store.go index ab14d7c6..3e31b369 100644 --- a/snapshot/store.go +++ b/snapshot/store.go @@ -326,7 +326,6 @@ func (s *Store) getSnapshots() ([]*raft.SnapshotMeta, error) { } // getDBPath returns the path to the database file for the most recent snapshot. -// It is mostly useful for testing. func (s *Store) getDBPath() (string, error) { snapshots, err := s.getSnapshots() if err != nil { diff --git a/snapshot/testdata/db-and-wals/empty-file b/snapshot/testdata/db-and-wals/empty-file new file mode 100644 index 00000000..e69de29b