From 9b6fc27c4b53405933dc683b40212ef3c1c23cea Mon Sep 17 00:00:00 2001 From: Sarthak Makhija Date: Mon, 22 May 2023 11:40:13 +0530 Subject: [PATCH 1/5] Sync active memtable and value log on Db.Sync (#1847) --- db.go | 38 ++++++++++++++++++++++++++- db2_test.go | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++ db_test.go | 61 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 1 deletion(-) diff --git a/db.go b/db.go index 30aa2c13a..491c60998 100644 --- a/db.go +++ b/db.go @@ -676,7 +676,43 @@ const ( // Sync syncs database content to disk. This function provides // more control to user to sync data whenever required. func (db *DB) Sync() error { - return db.vlog.sync() + /** + Make an attempt to sync both the logs, the active memtable's WAL and the vLog (1847). + Cases: + - All_ok :: If both the logs sync successfully. + + - Entry_Lost :: If an entry with a value pointer was present in the active memtable's WAL, + :: and the WAL was synced but there was an error in syncing the vLog. + :: The entry will be considered lost and this case will need to be handled during recovery. + + - Entries_Lost :: If there were errors in syncing both the logs, multiple entries would be lost. + + - Entries_Lost :: If the active memtable's WAL is not synced but the vLog is synced, it will + :: result in entries being lost because recovery of the active memtable is done from its WAL. + :: Check `UpdateSkipList` in memtable.go. + + - Nothing_lost :: If an entry with its value was present in the active memtable's WAL, and the WAL was synced, + :: but there was an error in syncing the vLog. + :: Nothing is lost for this very specific entry because the entry is completely present in the memtable's WAL. + + - Partially_lost :: If entries were written partially in either of the logs, the logs will be truncated during recovery. + :: As a result of truncation, some entries might be lost. Assume that 4KB of data is to be synced and + :: invoking `Sync` results only in syncing 3KB of data and then the machine shuts down or the disk failure happens, + :: this will result in partial writes. [[This case needs verification]] + */ + + var err error + + memtableSyncError := db.mt.SyncWAL() + if memtableSyncError != nil { + err = errors.Wrap(memtableSyncError, "sync failed for the active memtable WAL") + } + + vLogSyncError := db.vlog.sync() + if vLogSyncError != nil { + err = errors.Wrap(vLogSyncError, "sync failed for the vLog") + } + return err } // getMemtables returns the current memtables and get references. diff --git a/db2_test.go b/db2_test.go index 4474d51dc..2eaeb2051 100644 --- a/db2_test.go +++ b/db2_test.go @@ -1059,3 +1059,79 @@ func TestKeyCount(t *testing.T) { require.NoError(t, stream.Orchestrate(context.Background())) require.Equal(t, N, uint64(count)) } + +func TestAssertValueLogIsNotWrittenToOnStartup(t *testing.T) { + opt := DefaultOptions("").WithValueLogFileSize(1 << 20).WithValueThreshold(1 << 4) + + dir, err := os.MkdirTemp(".", "badger-test") + require.NoError(t, err) + defer removeDir(dir) + + openDb := func(readonly bool) *DB { + opts := &opt + opts.Dir = dir + opts.ValueDir = dir + if readonly { + opts.ReadOnly = true + } + + if opts.InMemory { + opts.Dir = "" + opts.ValueDir = "" + } + db, err := Open(*opts) + require.NoError(t, err) + + return db + } + + key := func(i int) string { + return fmt.Sprintf("key%100d", i) + } + + assertOnLoadDb := func(db *DB) uint32 { + data := []byte(fmt.Sprintf("value%100d", 1)) + for i := 0; i < 20; i++ { + err := db.Update(func(txn *Txn) error { + return txn.SetEntry(NewEntry([]byte(key(i)), data)) + }) + require.NoError(t, err) + } + return db.vlog.maxFid + } + + latestVLogFileSize := func(db *DB, vLogId uint32) uint32 { + return db.vlog.filesMap[vLogId].size.Load() + } + + assertOnReadDb := func(db *DB) { + for i := 0; i < 20; i++ { + err := db.View(func(txn *Txn) error { + item, err := txn.Get([]byte(key(i))) + require.NoError(t, err, "Getting key: %s", key(i)) + err = item.Value(func(v []byte) error { + _ = v + return nil + }) + require.NoError(t, err, "Getting value for the key: %s", key(i)) + return nil + }) + require.NoError(t, err) + } + } + + db := openDb(false) + vLogFileSize := latestVLogFileSize(db, assertOnLoadDb(db)) + assertOnReadDb(db) + + require.NoError(t, db.Sync()) + require.NoError(t, db.Close()) + + db = openDb(true) + defer func() { + require.NoError(t, db.Close()) + }() + + assertOnReadDb(db) + require.Equal(t, latestVLogFileSize(db, db.vlog.maxFid), vLogFileSize) +} diff --git a/db_test.go b/db_test.go index c299c152a..aa0c20487 100644 --- a/db_test.go +++ b/db_test.go @@ -2035,6 +2035,67 @@ func TestSyncForRace(t *testing.T) { <-doneChan } +func TestSyncForNoErrors(t *testing.T) { + dir, err := os.MkdirTemp("", "badger-test") + require.NoError(t, err) + defer removeDir(dir) + + db, err := Open(DefaultOptions(dir).WithSyncWrites(false)) + require.NoError(t, err) + defer func() { require.NoError(t, db.Close()) }() + + txn := db.NewTransaction(true) + for i := 0; i < 10; i++ { + require.NoError( + t, + txn.SetEntry(NewEntry( + []byte(fmt.Sprintf("key%d", i)), + []byte(fmt.Sprintf("value%d", i)), + )), + ) + } + require.NoError(t, txn.Commit()) + + if err := db.Sync(); err != nil { + require.NoError(t, err) + } +} + +func TestSyncForReadingTheEntriesThatWereSynced(t *testing.T) { + dir, err := os.MkdirTemp("", "badger-test") + require.NoError(t, err) + defer removeDir(dir) + + db, err := Open(DefaultOptions(dir).WithSyncWrites(false)) + require.NoError(t, err) + defer func() { require.NoError(t, db.Close()) }() + + txn := db.NewTransaction(true) + for i := 0; i < 10; i++ { + require.NoError( + t, + txn.SetEntry(NewEntry( + []byte(fmt.Sprintf("key%d", i)), + []byte(fmt.Sprintf("value%d", i)), + )), + ) + } + require.NoError(t, txn.Commit()) + + if err := db.Sync(); err != nil { + require.NoError(t, err) + } + + readOnlyTxn := db.NewTransaction(false) + for i := 0; i < 10; i++ { + item, err := readOnlyTxn.Get([]byte(fmt.Sprintf("key%d", i))) + require.NoError(t, err) + + value := getItemValue(t, item) + require.Equal(t, []byte(fmt.Sprintf("value%d", i)), value) + } +} + func TestForceFlushMemtable(t *testing.T) { dir, err := os.MkdirTemp("", "badger-test") require.NoError(t, err, "temp dir for badger could not be created") From dc82e956e941f2a3df1b5eda8375691fa082304c Mon Sep 17 00:00:00 2001 From: Sarthak Makhija Date: Mon, 22 May 2023 12:27:57 +0530 Subject: [PATCH 2/5] Add lock during DB.Sync operation and apply linting feedback from the CI (#1847) --- db.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/db.go b/db.go index 491c60998..8075f289f 100644 --- a/db.go +++ b/db.go @@ -695,19 +695,21 @@ func (db *DB) Sync() error { :: but there was an error in syncing the vLog. :: Nothing is lost for this very specific entry because the entry is completely present in the memtable's WAL. - - Partially_lost :: If entries were written partially in either of the logs, the logs will be truncated during recovery. - :: As a result of truncation, some entries might be lost. Assume that 4KB of data is to be synced and - :: invoking `Sync` results only in syncing 3KB of data and then the machine shuts down or the disk failure happens, - :: this will result in partial writes. [[This case needs verification]] + - Partially_lost :: If entries were written partially in either of the logs, + :: the logs will be truncated during recovery. + :: As a result of truncation, some entries might be lost. + :: Assume that 4KB of data is to be synced and invoking `Sync` results only in syncing 3KB + :: of data and then the machine shuts down or the disk failure happens, + :: this will result in partial writes. [[This case needs verification]] */ + db.lock.Lock() + defer db.lock.Unlock() var err error - memtableSyncError := db.mt.SyncWAL() if memtableSyncError != nil { err = errors.Wrap(memtableSyncError, "sync failed for the active memtable WAL") } - vLogSyncError := db.vlog.sync() if vLogSyncError != nil { err = errors.Wrap(vLogSyncError, "sync failed for the vLog") From ad59b778640f93670e6fee7fc35c6d394412f256 Mon Sep 17 00:00:00 2001 From: Sarthak Makhija Date: Mon, 22 May 2023 13:43:31 +0530 Subject: [PATCH 3/5] Correct the logic to combine memtable and vlog sync errors and add tests for the same (#1847) --- db.go | 15 ++++----------- y/error.go | 13 +++++++++++++ y/error_test.go | 27 +++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 11 deletions(-) create mode 100644 y/error_test.go diff --git a/db.go b/db.go index 8075f289f..1a018e1ce 100644 --- a/db.go +++ b/db.go @@ -702,19 +702,12 @@ func (db *DB) Sync() error { :: of data and then the machine shuts down or the disk failure happens, :: this will result in partial writes. [[This case needs verification]] */ - db.lock.Lock() - defer db.lock.Unlock() - - var err error + db.lock.RLock() memtableSyncError := db.mt.SyncWAL() - if memtableSyncError != nil { - err = errors.Wrap(memtableSyncError, "sync failed for the active memtable WAL") - } + db.lock.RUnlock() + vLogSyncError := db.vlog.sync() - if vLogSyncError != nil { - err = errors.Wrap(vLogSyncError, "sync failed for the vLog") - } - return err + return y.CombineErrors(memtableSyncError, vLogSyncError) } // getMemtables returns the current memtables and get references. diff --git a/y/error.go b/y/error.go index a727a82ea..95528d876 100644 --- a/y/error.go +++ b/y/error.go @@ -84,3 +84,16 @@ func Wrapf(err error, format string, args ...interface{}) error { } return errors.Wrapf(err, format, args...) } + +func CombineErrors(one, other error) error { + if one != nil && other != nil { + return fmt.Errorf("%w; %w", one, other) + } + if one != nil && other == nil { + return fmt.Errorf("%w", one) + } + if one == nil && other != nil { + return fmt.Errorf("%w", other) + } + return nil +} diff --git a/y/error_test.go b/y/error_test.go new file mode 100644 index 000000000..61ea6a4c6 --- /dev/null +++ b/y/error_test.go @@ -0,0 +1,27 @@ +package y + +import ( + "errors" + "github.com/stretchr/testify/require" + "testing" +) + +func TestCombineWithBothErrorsPresent(t *testing.T) { + combinedError := CombineErrors(errors.New("one"), errors.New("two")) + require.Equal(t, "one; two", combinedError.Error()) +} + +func TestCombineErrorsWithOneErrorPresent(t *testing.T) { + combinedError := CombineErrors(errors.New("one"), nil) + require.Equal(t, "one", combinedError.Error()) +} + +func TestCombineErrorsWithOtherErrorPresent(t *testing.T) { + combinedError := CombineErrors(nil, errors.New("other")) + require.Equal(t, "other", combinedError.Error()) +} + +func TestCombineErrorsWithBothErrorsAsNil(t *testing.T) { + combinedError := CombineErrors(nil, nil) + require.NoError(t, combinedError) +} From 9f81eceffdcbb22e6deb7915f3d36d0b5c046054 Mon Sep 17 00:00:00 2001 From: Sarthak Makhija Date: Mon, 22 May 2023 14:16:02 +0530 Subject: [PATCH 4/5] Changed the format specifier in CombineErrors, internal error details are not revealed (#1847) --- y/error.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/y/error.go b/y/error.go index 95528d876..f36181b46 100644 --- a/y/error.go +++ b/y/error.go @@ -87,13 +87,13 @@ func Wrapf(err error, format string, args ...interface{}) error { func CombineErrors(one, other error) error { if one != nil && other != nil { - return fmt.Errorf("%w; %w", one, other) + return fmt.Errorf("%v; %v", one, other) } if one != nil && other == nil { - return fmt.Errorf("%w", one) + return fmt.Errorf("%v", one) } if one == nil && other != nil { - return fmt.Errorf("%w", other) + return fmt.Errorf("%v", other) } return nil } From 70ffdb0b8733295be39759c99ea1621ea2d18934 Mon Sep 17 00:00:00 2001 From: Sarthak Makhija Date: Mon, 22 May 2023 19:06:24 +0530 Subject: [PATCH 5/5] Sarthak | Changed the ordering of the import packages (#1847) --- y/error_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/y/error_test.go b/y/error_test.go index 61ea6a4c6..b8091dc2e 100644 --- a/y/error_test.go +++ b/y/error_test.go @@ -2,8 +2,9 @@ package y import ( "errors" - "github.com/stretchr/testify/require" "testing" + + "github.com/stretchr/testify/require" ) func TestCombineWithBothErrorsPresent(t *testing.T) {