From 54aec1ca33e13610cd4d75dfd3fe865bbd473d21 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Wed, 19 Oct 2022 20:38:25 -0400 Subject: [PATCH 1/5] Tighten up backup code --- db/db.go | 4 ++-- store/store.go | 55 +++++++++++++++++++++----------------------------- 2 files changed, 25 insertions(+), 34 deletions(-) diff --git a/db/db.go b/db/db.go index 47cc8a11..d7c48162 100644 --- a/db/db.go +++ b/db/db.go @@ -674,8 +674,8 @@ func (db *DB) Copy(dstDB *DB) error { // an ordinary on-disk database file, the serialization is just a copy of the // disk file. For an in-memory database or a "TEMP" database, the serialization // is the same sequence of bytes which would be written to disk if that database -// were backed up to disk. This function must not be called while any transaction -// is in progress. +// were backed up to disk. This function must not be called while any writes +// are happening to the database. func (db *DB) Serialize() ([]byte, error) { if !db.memory { // Simply read and return the SQLite file. diff --git a/store/store.go b/store/store.go index 7650d6fc..fcaa39c8 100644 --- a/store/store.go +++ b/store/store.go @@ -810,16 +810,35 @@ func (s *Store) Query(qr *command.QueryRequest) ([]*command.QueryRows, error) { // Backup writes a snapshot of the underlying database to dst // // If Leader is true for the request, this operation is performed with a read consistency -// level equivalent to "weak". Otherwise, no guarantees are made about the read consistency level. +// level equivalent to "weak". Otherwise, no guarantees are made about the read consistency +// level. This function is safe to call while the database is being changed. func (s *Store) Backup(br *command.BackupRequest, dst io.Writer) error { if br.Leader && s.raft.State() != raft.Leader { return ErrNotLeader } if br.Format == command.BackupRequest_BACKUP_REQUEST_FORMAT_BINARY { - if err := s.database(br.Leader, dst); err != nil { + f, err := ioutil.TempFile("", "rqlilte-snap-") + if err != nil { + return err + } + if err := f.Close(); err != nil { + return err + } + + if err := s.db.Backup(f.Name()); err != nil { + return err + } + + of, err := os.Open(f.Name()) + if err != nil { return err } + defer of.Close() + defer os.Remove(f.Name()) + + _, err = io.Copy(dst, of) + return err } else if br.Format == command.BackupRequest_BACKUP_REQUEST_FORMAT_SQL { if err := s.db.Dump(dst); err != nil { return err @@ -1151,7 +1170,7 @@ func (s *Store) Apply(l *raft.Log) (e interface{}) { // about the read consistency level. // // http://sqlite.org/howtocorrupt.html states it is safe to do this -// as long as no transaction is in progress. +// as long as the database is not written to during the call. func (s *Store) Database(leader bool) ([]byte, error) { if leader && s.raft.State() != raft.Leader { return nil, ErrNotLeader @@ -1168,7 +1187,7 @@ func (s *Store) Database(leader bool) ([]byte, error) { // However, queries that involve a transaction must be blocked. // // http://sqlite.org/howtocorrupt.html states it is safe to copy or serialize the -// database as long as no transaction is in progress. +// database as long as no writes to the database are in progress. func (s *Store) Snapshot() (raft.FSMSnapshot, error) { defer func() { s.numSnapshotsMu.Lock() @@ -1290,34 +1309,6 @@ func (s *Store) logSize() (int64, error) { return fi.Size(), nil } -// Database copies contents of the underlying SQLite database to dst -func (s *Store) database(leader bool, dst io.Writer) error { - if leader && s.raft.State() != raft.Leader { - return ErrNotLeader - } - - f, err := ioutil.TempFile("", "rqlilte-snap-") - if err != nil { - return err - } - if err := f.Close(); err != nil { - return err - } - - if err := s.db.Backup(f.Name()); err != nil { - return err - } - - of, err := os.Open(f.Name()) - if err != nil { - return err - } - defer of.Close() - - _, err = io.Copy(dst, of) - return err -} - func (s *Store) databaseTypePretty() string { if s.dbConf.Memory { return "in-memory" From 7e8e2dd3e8ad66fe6acf6e7f0017dbdf52b475c0 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Wed, 19 Oct 2022 20:41:42 -0400 Subject: [PATCH 2/5] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a33ae5d8..377fe549 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ### Implementation changes and bug fixes - [PR #1079](https://github.com/rqlite/rqlite/pull/1079): Use a Protobuf model for Backup requests. - [PR #1078](https://github.com/rqlite/rqlite/pull/1078): Decrease bootstrap polling interval from 5 seconds to 2 seconds. +- [PR #1082](https://github.com/rqlite/rqlite/pull/1082): Small refactor of backup code. ## 7.7.2 (October 14th 2022) ### Implementation changes and bug fixes From 555883d1a223e02046b4ad2de75e63f50ae2fce2 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Wed, 19 Oct 2022 20:43:06 -0400 Subject: [PATCH 3/5] Correct order of deferred calls --- store/store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/store.go b/store/store.go index fcaa39c8..3e3ceadc 100644 --- a/store/store.go +++ b/store/store.go @@ -834,8 +834,8 @@ func (s *Store) Backup(br *command.BackupRequest, dst io.Writer) error { if err != nil { return err } - defer of.Close() defer os.Remove(f.Name()) + defer of.Close() _, err = io.Copy(dst, of) return err From 9fc6ea16e986c54f60ca423eb0f39b4a45d3c1b0 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Wed, 19 Oct 2022 20:44:58 -0400 Subject: [PATCH 4/5] Even better defer sequence --- store/store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/store/store.go b/store/store.go index 3e3ceadc..6e3aebe7 100644 --- a/store/store.go +++ b/store/store.go @@ -825,6 +825,7 @@ func (s *Store) Backup(br *command.BackupRequest, dst io.Writer) error { if err := f.Close(); err != nil { return err } + defer os.Remove(f.Name()) if err := s.db.Backup(f.Name()); err != nil { return err @@ -834,7 +835,6 @@ func (s *Store) Backup(br *command.BackupRequest, dst io.Writer) error { if err != nil { return err } - defer os.Remove(f.Name()) defer of.Close() _, err = io.Copy(dst, of) From 47d79c859ae7ce7ee64035a12d6cbb505d5bdda9 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Wed, 19 Oct 2022 20:55:05 -0400 Subject: [PATCH 5/5] Log database backup time --- store/store.go | 20 +++++++++++--------- store/store_test.go | 2 +- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/store/store.go b/store/store.go index 6e3aebe7..e322b0a2 100644 --- a/store/store.go +++ b/store/store.go @@ -812,7 +812,15 @@ func (s *Store) Query(qr *command.QueryRequest) ([]*command.QueryRows, error) { // If Leader is true for the request, this operation is performed with a read consistency // level equivalent to "weak". Otherwise, no guarantees are made about the read consistency // level. This function is safe to call while the database is being changed. -func (s *Store) Backup(br *command.BackupRequest, dst io.Writer) error { +func (s *Store) Backup(br *command.BackupRequest, dst io.Writer) (retErr error) { + startT := time.Now() + defer func() { + if retErr == nil { + stats.Add(numBackups, 1) + s.logger.Printf("database backed up in %s", time.Since(startT)) + } + }() + if br.Leader && s.raft.State() != raft.Leader { return ErrNotLeader } @@ -840,15 +848,9 @@ func (s *Store) Backup(br *command.BackupRequest, dst io.Writer) error { _, err = io.Copy(dst, of) return err } else if br.Format == command.BackupRequest_BACKUP_REQUEST_FORMAT_SQL { - if err := s.db.Dump(dst); err != nil { - return err - } - } else { - return ErrInvalidBackupFormat + return s.db.Dump(dst) } - - stats.Add(numBackups, 1) - return nil + return ErrInvalidBackupFormat } // Loads an entire SQLite file into the database, sending the request diff --git a/store/store_test.go b/store/store_test.go index cb0bfa48..06d33fc4 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -474,7 +474,7 @@ COMMIT; f, err := ioutil.TempFile("", "rqlite-baktest-") defer os.Remove(f.Name()) - s.logger.Printf("backup file is %s", f.Name()) + t.Logf("backup file is %s", f.Name()) if err := s.Backup(backupRequestBinary(true), f); err != nil { t.Fatalf("Backup failed %s", err.Error())