From 0ccb16dc02e7dbf984a1e0dce92c01b01b062b45 Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Tue, 20 Dec 2022 15:51:49 +0100 Subject: [PATCH 1/7] Mechanical move of Check functions to tx_check.go file. Signed-off-by: Piotr Tabor --- tx.go | 92 --------------------------------------------------- tx_check.go | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 92 deletions(-) create mode 100644 tx_check.go diff --git a/tx.go b/tx.go index 97adbe762..4c605a238 100644 --- a/tx.go +++ b/tx.go @@ -401,98 +401,6 @@ func (tx *Tx) CopyFile(path string, mode os.FileMode) error { return f.Close() } -// Check performs several consistency checks on the database for this transaction. -// An error is returned if any inconsistency is found. -// -// It can be safely run concurrently on a writable transaction. However, this -// incurs a high cost for large databases and databases with a lot of subbuckets -// because of caching. This overhead can be removed if running on a read-only -// transaction, however, it is not safe to execute other writer transactions at -// the same time. -func (tx *Tx) Check() <-chan error { - ch := make(chan error) - go tx.check(ch) - return ch -} - -func (tx *Tx) check(ch chan error) { - // Force loading free list if opened in ReadOnly mode. - tx.db.loadFreelist() - - // Check if any pages are double freed. - freed := make(map[pgid]bool) - all := make([]pgid, tx.db.freelist.count()) - tx.db.freelist.copyall(all) - for _, id := range all { - if freed[id] { - ch <- fmt.Errorf("page %d: already freed", id) - } - freed[id] = true - } - - // Track every reachable page. - reachable := make(map[pgid]*page) - reachable[0] = tx.page(0) // meta0 - reachable[1] = tx.page(1) // meta1 - if tx.meta.freelist != pgidNoFreelist { - for i := uint32(0); i <= tx.page(tx.meta.freelist).overflow; i++ { - reachable[tx.meta.freelist+pgid(i)] = tx.page(tx.meta.freelist) - } - } - - // Recursively check buckets. - tx.checkBucket(&tx.root, reachable, freed, ch) - - // Ensure all pages below high water mark are either reachable or freed. - for i := pgid(0); i < tx.meta.pgid; i++ { - _, isReachable := reachable[i] - if !isReachable && !freed[i] { - ch <- fmt.Errorf("page %d: unreachable unfreed", int(i)) - } - } - - // Close the channel to signal completion. - close(ch) -} - -func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bool, ch chan error) { - // Ignore inline buckets. - if b.root == 0 { - return - } - - // Check every page used by this bucket. - b.tx.forEachPage(b.root, func(p *page, _ int, stack []pgid) { - if p.id > tx.meta.pgid { - ch <- fmt.Errorf("page %d: out of bounds: %d (stack: %v)", int(p.id), int(b.tx.meta.pgid), stack) - } - - // Ensure each page is only referenced once. - for i := pgid(0); i <= pgid(p.overflow); i++ { - var id = p.id + i - if _, ok := reachable[id]; ok { - ch <- fmt.Errorf("page %d: multiple references (stack: %v)", int(id), stack) - } - reachable[id] = p - } - - // We should only encounter un-freed leaf and branch pages. - if freed[p.id] { - ch <- fmt.Errorf("page %d: reachable freed", int(p.id)) - } else if (p.flags&branchPageFlag) == 0 && (p.flags&leafPageFlag) == 0 { - ch <- fmt.Errorf("page %d: invalid type: %s (stack: %v)", int(p.id), p.typ(), stack) - } - }) - - // Check each bucket within this bucket. - _ = b.ForEachBucket(func(k []byte) error { - if child := b.Bucket(k); child != nil { - tx.checkBucket(child, reachable, freed, ch) - } - return nil - }) -} - // allocate returns a contiguous block of memory starting at a given page. func (tx *Tx) allocate(count int) (*page, error) { p, err := tx.db.allocate(tx.meta.txid, count) diff --git a/tx_check.go b/tx_check.go new file mode 100644 index 000000000..84d3eb5dc --- /dev/null +++ b/tx_check.go @@ -0,0 +1,95 @@ +package bbolt + +import "fmt" + +// Check performs several consistency checks on the database for this transaction. +// An error is returned if any inconsistency is found. +// +// It can be safely run concurrently on a writable transaction. However, this +// incurs a high cost for large databases and databases with a lot of subbuckets +// because of caching. This overhead can be removed if running on a read-only +// transaction, however, it is not safe to execute other writer transactions at +// the same time. +func (tx *Tx) Check() <-chan error { + ch := make(chan error) + go tx.check(ch) + return ch +} + +func (tx *Tx) check(ch chan error) { + // Force loading free list if opened in ReadOnly mode. + tx.db.loadFreelist() + + // Check if any pages are double freed. + freed := make(map[pgid]bool) + all := make([]pgid, tx.db.freelist.count()) + tx.db.freelist.copyall(all) + for _, id := range all { + if freed[id] { + ch <- fmt.Errorf("page %d: already freed", id) + } + freed[id] = true + } + + // Track every reachable page. + reachable := make(map[pgid]*page) + reachable[0] = tx.page(0) // meta0 + reachable[1] = tx.page(1) // meta1 + if tx.meta.freelist != pgidNoFreelist { + for i := uint32(0); i <= tx.page(tx.meta.freelist).overflow; i++ { + reachable[tx.meta.freelist+pgid(i)] = tx.page(tx.meta.freelist) + } + } + + // Recursively check buckets. + tx.checkBucket(&tx.root, reachable, freed, ch) + + // Ensure all pages below high water mark are either reachable or freed. + for i := pgid(0); i < tx.meta.pgid; i++ { + _, isReachable := reachable[i] + if !isReachable && !freed[i] { + ch <- fmt.Errorf("page %d: unreachable unfreed", int(i)) + } + } + + // Close the channel to signal completion. + close(ch) +} + +func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bool, ch chan error) { + // Ignore inline buckets. + if b.root == 0 { + return + } + + // Check every page used by this bucket. + b.tx.forEachPage(b.root, func(p *page, _ int, stack []pgid) { + if p.id > tx.meta.pgid { + ch <- fmt.Errorf("page %d: out of bounds: %d (stack: %v)", int(p.id), int(b.tx.meta.pgid), stack) + } + + // Ensure each page is only referenced once. + for i := pgid(0); i <= pgid(p.overflow); i++ { + var id = p.id + i + if _, ok := reachable[id]; ok { + ch <- fmt.Errorf("page %d: multiple references (stack: %v)", int(id), stack) + } + reachable[id] = p + } + + // We should only encounter un-freed leaf and branch pages. + if freed[p.id] { + ch <- fmt.Errorf("page %d: reachable freed", int(p.id)) + } else if (p.flags&branchPageFlag) == 0 && (p.flags&leafPageFlag) == 0 { + ch <- fmt.Errorf("page %d: invalid type: %s (stack: %v)", int(p.id), p.typ(), stack) + } + }) + + // Check each bucket within this bucket. + _ = b.ForEachBucket(func(k []byte) error { + if child := b.Bucket(k); child != nil { + tx.checkBucket(child, reachable, freed, ch) + } + return nil + }) +} From 0c8d75db1e41e5e3bc97ba4b8227eac22bae58bc Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Tue, 20 Dec 2022 15:19:54 +0100 Subject: [PATCH 2/7] Recursive checker implementation. Recursive checker confirms database consistency with respect to b-tree key order constraints: - keys on pages must be sorted - keys on children pages are between 2 consecutive keys on parent branch page). Signed-off-by: Piotr Tabor --- cmd/bbolt/main.go | 24 ++++++- db.go | 4 +- db_test.go | 4 +- internal/btesting/btesting.go | 2 +- node.go | 4 ++ tx.go | 2 +- tx_check.go | 121 ++++++++++++++++++++++++++++++++-- tx_test.go | 3 +- 8 files changed, 150 insertions(+), 14 deletions(-) diff --git a/cmd/bbolt/main.go b/cmd/bbolt/main.go index ff24df019..8bca086db 100644 --- a/cmd/bbolt/main.go +++ b/cmd/bbolt/main.go @@ -207,7 +207,7 @@ func (cmd *CheckCommand) Run(args ...string) error { // Perform consistency check. return db.View(func(tx *bolt.Tx) error { var count int - for err := range tx.Check() { + for err := range tx.Check(CmdKeyValueStringer()) { fmt.Fprintln(cmd.Stdout, err) count++ } @@ -1689,3 +1689,25 @@ Additional options include: Defaults to 64KB. `, "\n") } + +type cmdKeyValueStringer struct{} + +func (_ cmdKeyValueStringer) KeyToString(key []byte) string { + if isPrintable(string(key)) { + return string(key) + } else { + return hex.EncodeToString(key) + } +} + +func (_ cmdKeyValueStringer) ValueToString(value []byte) string { + if isPrintable(string(value)) { + return string(value) + } else { + return hex.EncodeToString(value) + } +} + +func CmdKeyValueStringer() bolt.KeyValueStringer { + return cmdKeyValueStringer{} +} diff --git a/db.go b/db.go index d9749f46e..ed3c88681 100644 --- a/db.go +++ b/db.go @@ -1148,9 +1148,11 @@ func (db *DB) freepages() []pgid { panic(fmt.Sprintf("freepages: failed to get all reachable pages (%v)", e)) } }() - tx.checkBucket(&tx.root, reachable, nofreed, ech) + tx.checkBucket(&tx.root, reachable, nofreed, HexKeyValueStringer(), ech) close(ech) + // TODO: If check bucket reported any corruptions (ech) we shouldn't proceed to freeing the pages. + var fids []pgid for i := pgid(2); i < db.meta().pgid; i++ { if _, ok := reachable[i]; !ok { diff --git a/db_test.go b/db_test.go index 3d9f229e3..c7ce9cee0 100644 --- a/db_test.go +++ b/db_test.go @@ -396,7 +396,7 @@ func TestOpen_Check(t *testing.T) { if err != nil { t.Fatal(err) } - if err = db.View(func(tx *bolt.Tx) error { return <-tx.Check() }); err != nil { + if err = db.View(func(tx *bolt.Tx) error { return <-tx.Check(bolt.HexKeyValueStringer()) }); err != nil { t.Fatal(err) } if err = db.Close(); err != nil { @@ -407,7 +407,7 @@ func TestOpen_Check(t *testing.T) { if err != nil { t.Fatal(err) } - if err := db.View(func(tx *bolt.Tx) error { return <-tx.Check() }); err != nil { + if err := db.View(func(tx *bolt.Tx) error { return <-tx.Check(bolt.HexKeyValueStringer()) }); err != nil { t.Fatal(err) } if err := db.Close(); err != nil { diff --git a/internal/btesting/btesting.go b/internal/btesting/btesting.go index b30507234..42ec2b14f 100644 --- a/internal/btesting/btesting.go +++ b/internal/btesting/btesting.go @@ -119,7 +119,7 @@ func (db *DB) MustCheck() { err := db.Update(func(tx *bolt.Tx) error { // Collect all the errors. var errors []error - for err := range tx.Check() { + for err := range tx.Check(bolt.HexKeyValueStringer()) { errors = append(errors, err) if len(errors) > 10 { break diff --git a/node.go b/node.go index b5ddce619..321536bd9 100644 --- a/node.go +++ b/node.go @@ -585,6 +585,10 @@ func (n *node) dump() { } */ +func compareKeys(left, right []byte) int { + return bytes.Compare(left, right) +} + type nodes []*node func (s nodes) Len() int { return len(s) } diff --git a/tx.go b/tx.go index 4c605a238..e9f13b55d 100644 --- a/tx.go +++ b/tx.go @@ -190,7 +190,7 @@ func (tx *Tx) Commit() error { // If strict mode is enabled then perform a consistency check. if tx.db.StrictMode { - ch := tx.Check() + ch := tx.Check(HexKeyValueStringer()) var errs []string for { err, ok := <-ch diff --git a/tx_check.go b/tx_check.go index 84d3eb5dc..350cbf278 100644 --- a/tx_check.go +++ b/tx_check.go @@ -1,6 +1,9 @@ package bbolt -import "fmt" +import ( + "encoding/hex" + "fmt" +) // Check performs several consistency checks on the database for this transaction. // An error is returned if any inconsistency is found. @@ -10,13 +13,13 @@ import "fmt" // because of caching. This overhead can be removed if running on a read-only // transaction, however, it is not safe to execute other writer transactions at // the same time. -func (tx *Tx) Check() <-chan error { +func (tx *Tx) Check(keyValueStringer KeyValueStringer) <-chan error { ch := make(chan error) - go tx.check(ch) + go tx.check(keyValueStringer, ch) return ch } -func (tx *Tx) check(ch chan error) { +func (tx *Tx) check(keyValueStringer KeyValueStringer, ch chan error) { // Force loading free list if opened in ReadOnly mode. tx.db.loadFreelist() @@ -42,7 +45,7 @@ func (tx *Tx) check(ch chan error) { } // Recursively check buckets. - tx.checkBucket(&tx.root, reachable, freed, ch) + tx.checkBucket(&tx.root, reachable, freed, keyValueStringer, ch) // Ensure all pages below high water mark are either reachable or freed. for i := pgid(0); i < tx.meta.pgid; i++ { @@ -56,7 +59,8 @@ func (tx *Tx) check(ch chan error) { close(ch) } -func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bool, ch chan error) { +func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bool, + keyValueStringer KeyValueStringer, ch chan error) { // Ignore inline buckets. if b.root == 0 { return @@ -85,11 +89,114 @@ func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bo } }) + tx.recursivelyCheckPages(b.root, keyValueStringer.KeyToString, ch) + // Check each bucket within this bucket. _ = b.ForEachBucket(func(k []byte) error { if child := b.Bucket(k); child != nil { - tx.checkBucket(child, reachable, freed, ch) + tx.checkBucket(child, reachable, freed, keyValueStringer, ch) } return nil }) } + +// recursivelyCheckPages confirms database consistency with respect to b-tree +// key order constraints: +// - keys on pages must be sorted +// - keys on children pages are between 2 consecutive keys on the parent's branch page). +func (tx *Tx) recursivelyCheckPages(pgid pgid, keyToString func([]byte) string, ch chan error) { + tx.recursivelyCheckPagesInternal(pgid, nil, nil, nil, keyToString, ch) +} + +// recursivelyCheckPagesInternal verifies that all keys in the subtree rooted at `pgid` are: +// - >=`minKeyClosed` (can be nil) +// - <`maxKeyOpen` (can be nil) +// - Are in right ordering relationship to their parents. +// `pagesStack` is expected to contain IDs of pages from the tree root to `pgid` for the clean debugging message. +func (tx *Tx) recursivelyCheckPagesInternal( + pgid pgid, minKeyClosed, maxKeyOpen []byte, pagesStack []pgid, + keyToString func([]byte) string, ch chan error) (maxKeyInSubtree []byte) { + + p := tx.page(pgid) + pagesStack = append(pagesStack, pgid) + switch { + case p.flags&branchPageFlag != 0: + // For branch page we navigate ranges of all subpages. + runningMin := minKeyClosed + for i := range p.branchPageElements() { + elem := p.branchPageElement(uint16(i)) + if i == 0 && runningMin != nil && compareKeys(runningMin, elem.key()) > 0 { + ch <- fmt.Errorf("key (%d, %s) on the branch page(%d) needs to be >="+ + " to the index in the ancestor. Pages stack: %v", + i, keyToString(elem.key()), pgid, pagesStack) + } + + if maxKeyOpen != nil && compareKeys(elem.key(), maxKeyOpen) >= 0 { + ch <- fmt.Errorf("key (%d: %s) on the branch page(%d) needs to be <"+ + " than key of the next element reachable from the ancestor (%v). Pages stack: %v", + i, keyToString(elem.key()), pgid, keyToString(maxKeyOpen), pagesStack) + } + + var maxKey []byte + if i < len(p.branchPageElements())-1 { + maxKey = p.branchPageElement(uint16(i + 1)).key() + } else { + maxKey = maxKeyOpen + } + maxKeyInSubtree = tx.recursivelyCheckPagesInternal(elem.pgid, elem.key(), maxKey, pagesStack, keyToString, ch) + runningMin = maxKeyInSubtree + } + return + case p.flags&leafPageFlag != 0: + runningMin := minKeyClosed + for i := range p.leafPageElements() { + elem := p.leafPageElement(uint16(i)) + if i == 0 && runningMin != nil && compareKeys(runningMin, elem.key()) > 0 { + ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be >= to the key in the ancestor. Stack: %v", + i, keyToString(elem.key()), pgid, pagesStack) + } + if i > 0 && compareKeys(runningMin, elem.key()) > 0 { + ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found <) than previous element (hex)%s. Stack: %v", + i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) + } + if i > 0 && compareKeys(runningMin, elem.key()) == 0 { + ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found =) than previous element (hex)%s. Stack: %v", + i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) + } + if maxKeyOpen != nil && compareKeys(elem.key(), maxKeyOpen) >= 0 { + ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be < than key of the next element in ancestor (hex)%s. Pages stack: %v", + i, keyToString(elem.key()), pgid, keyToString(maxKeyOpen), pagesStack) + } + runningMin = elem.key() + } + if p.count > 0 { + return p.leafPageElement(p.count - 1).key() + } + default: + ch <- fmt.Errorf("unexpected page type for pgid:%d", pgid) + } + return nil +} + +// =========================================================================================== + +// KeyValueStringer allows to prepare human-readable diagnostic messages. +type KeyValueStringer interface { + KeyToString([]byte) string + ValueToString([]byte) string +} + +// HexKeyValueStringer serializes both key & value to hex representation. +func HexKeyValueStringer() KeyValueStringer { + return hexKeyValueStringer{} +} + +type hexKeyValueStringer struct{} + +func (_ hexKeyValueStringer) KeyToString(key []byte) string { + return hex.EncodeToString(key) +} + +func (_ hexKeyValueStringer) ValueToString(value []byte) string { + return hex.EncodeToString(value) +} diff --git a/tx_test.go b/tx_test.go index 8b1b46c3e..9d2f0422d 100644 --- a/tx_test.go +++ b/tx_test.go @@ -48,7 +48,8 @@ func TestTx_Check_ReadOnly(t *testing.T) { numChecks := 2 errc := make(chan error, numChecks) check := func() { - errc <- <-tx.Check() + err, _ := <-tx.Check(bolt.HexKeyValueStringer()) + errc <- err } // Ensure the freelist is not reloaded and does not race. for i := 0; i < numChecks; i++ { From 710c33fe89f351471a717a5b1e465d577489ddb1 Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Tue, 20 Dec 2022 15:46:29 +0100 Subject: [PATCH 3/7] Tests for recursive checker (working on a corrupted files). Signed-off-by: Piotr Tabor --- internal/tests/tx_check_test.go | 85 +++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 internal/tests/tx_check_test.go diff --git a/internal/tests/tx_check_test.go b/internal/tests/tx_check_test.go new file mode 100644 index 000000000..7ba89ef64 --- /dev/null +++ b/internal/tests/tx_check_test.go @@ -0,0 +1,85 @@ +package tests_test + +import ( + "fmt" + "github.com/stretchr/testify/assert" + bolt "go.etcd.io/bbolt" + "go.etcd.io/bbolt/internal/btesting" + "go.etcd.io/bbolt/internal/guts_cli" + "go.etcd.io/bbolt/internal/surgeon" + "testing" +) + +func TestTx_RecursivelyCheckPages_MisplacedPage(t *testing.T) { + db := btesting.MustCreateDB(t) + assert.NoError(t, + db.Fill([]byte("data"), 1, 10000, + func(tx int, k int) []byte { return []byte(fmt.Sprintf("%04d", k)) }, + func(tx int, k int) []byte { return make([]byte, 100) }, + )) + assert.NoError(t, db.Close()) + + navigator := surgeon.NewXRay(db.Path()) + + path1, err := navigator.FindPathToPagesWithKey([]byte("0451")) + assert.NoError(t, err, "Cannot find page that contains key:'0451'") + assert.Len(t, path1, 1, "Expected only one page that contains key:'0451'") + + path2, err := navigator.FindPathToPagesWithKey([]byte("7563")) + assert.NoError(t, err, "Cannot find page that contains key:'7563'") + assert.Len(t, path2, 1, "Expected only one page that contains key:'7563'") + + srcPage := path1[0][len(path1[0])-1] + targetPage := path2[0][len(path2[0])-1] + assert.NoError(t, surgeon.CopyPage(db.Path(), srcPage, targetPage)) + + db.MustReopen() + assert.NoError(t, db.Update(func(tx *bolt.Tx) error { + // Collect all the errors. + var errors []error + for err := range tx.Check(bolt.HexKeyValueStringer()) { + errors = append(errors, err) + } + assert.Len(t, errors, 1) + assert.ErrorContains(t, errors[0], fmt.Sprintf("leaf page(%v) needs to be >= to the key in the ancestor", targetPage)) + return nil + })) + assert.NoError(t, db.Close()) +} + +func TestTx_RecursivelyCheckPages_CorruptedLeaf(t *testing.T) { + db := btesting.MustCreateDB(t) + assert.NoError(t, + db.Fill([]byte("data"), 1, 10000, + func(tx int, k int) []byte { return []byte(fmt.Sprintf("%04d", k)) }, + func(tx int, k int) []byte { return make([]byte, 100) }, + )) + assert.NoError(t, db.Close()) + + navigator := surgeon.NewXRay(db.Path()) + + path1, err := navigator.FindPathToPagesWithKey([]byte("0451")) + assert.NoError(t, err, "Cannot find page that contains key:'0451'") + assert.Len(t, path1, 1, "Expected only one page that contains key:'0451'") + + srcPage := path1[0][len(path1[0])-1] + p, pbuf, err := guts_cli.ReadPage(db.Path(), uint64(srcPage)) + assert.NoError(t, err) + assert.Positive(t, p.Count(), "page must be not empty") + p.LeafPageElement(p.Count() / 2).Key()[0] = 'z' + assert.NoError(t, surgeon.WritePage(db.Path(), pbuf)) + + db.MustReopen() + assert.NoError(t, db.Update(func(tx *bolt.Tx) error { + // Collect all the errors. + var errors []error + for err := range tx.Check(bolt.HexKeyValueStringer()) { + errors = append(errors, err) + } + assert.Len(t, errors, 2) + assert.ErrorContains(t, errors[0], fmt.Sprintf("leaf page(%v) needs to be < than key of the next element in ancestor", srcPage)) + assert.ErrorContains(t, errors[1], fmt.Sprintf("leaf page(%v) needs to be > (found <) than previous element", srcPage)) + return nil + })) + assert.NoError(t, db.Close()) +} From ee27a544ca071b12a67d7a1aec710bedff00bd5a Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Wed, 4 Jan 2023 10:06:03 +0100 Subject: [PATCH 4/7] Recursive checker: Final touches. Signed-off-by: Piotr Tabor --- go.mod | 2 -- go.sum | 4 ---- internal/tests/tx_check_test.go | 14 ++++++++------ scripts/fix.sh | 1 + tx_test.go | 3 +-- 5 files changed, 10 insertions(+), 14 deletions(-) diff --git a/go.mod b/go.mod index dd9fd703b..a58befa15 100644 --- a/go.mod +++ b/go.mod @@ -10,7 +10,5 @@ require ( require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - golang.org/x/mod v0.7.0 // indirect - golang.org/x/tools v0.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 1d1d2b208..3d9dedd3e 100644 --- a/go.sum +++ b/go.sum @@ -10,12 +10,8 @@ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKsk= github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -golang.org/x/mod v0.7.0 h1:LapD9S96VoQRhi/GrNTqeBJFrUjs5UHCAtTlgwA5oZA= -golang.org/x/mod v0.7.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/sys v0.3.0 h1:w8ZOecv6NaNa/zC8944JTU3vz4u6Lagfk4RPQxv92NQ= golang.org/x/sys v0.3.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/tools v0.4.0 h1:7mTAgkunk3fr4GAloyyCasadO6h9zSsQZbwvcaIciV4= -golang.org/x/tools v0.4.0/go.mod h1:UE5sM2OK9E/d67R0ANs2xJizIymRP5gJU295PvKXxjQ= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/tests/tx_check_test.go b/internal/tests/tx_check_test.go index 7ba89ef64..7e9cc286a 100644 --- a/internal/tests/tx_check_test.go +++ b/internal/tests/tx_check_test.go @@ -2,12 +2,14 @@ package tests_test import ( "fmt" + "testing" + "github.com/stretchr/testify/assert" + bolt "go.etcd.io/bbolt" "go.etcd.io/bbolt/internal/btesting" "go.etcd.io/bbolt/internal/guts_cli" "go.etcd.io/bbolt/internal/surgeon" - "testing" ) func TestTx_RecursivelyCheckPages_MisplacedPage(t *testing.T) { @@ -19,13 +21,13 @@ func TestTx_RecursivelyCheckPages_MisplacedPage(t *testing.T) { )) assert.NoError(t, db.Close()) - navigator := surgeon.NewXRay(db.Path()) + xRay := surgeon.NewXRay(db.Path()) - path1, err := navigator.FindPathToPagesWithKey([]byte("0451")) + path1, err := xRay.FindPathsToKey([]byte("0451")) assert.NoError(t, err, "Cannot find page that contains key:'0451'") assert.Len(t, path1, 1, "Expected only one page that contains key:'0451'") - path2, err := navigator.FindPathToPagesWithKey([]byte("7563")) + path2, err := xRay.FindPathsToKey([]byte("7563")) assert.NoError(t, err, "Cannot find page that contains key:'7563'") assert.Len(t, path2, 1, "Expected only one page that contains key:'7563'") @@ -56,9 +58,9 @@ func TestTx_RecursivelyCheckPages_CorruptedLeaf(t *testing.T) { )) assert.NoError(t, db.Close()) - navigator := surgeon.NewXRay(db.Path()) + xray := surgeon.NewXRay(db.Path()) - path1, err := navigator.FindPathToPagesWithKey([]byte("0451")) + path1, err := xray.FindPathsToKey([]byte("0451")) assert.NoError(t, err, "Cannot find page that contains key:'0451'") assert.Len(t, path1, 1, "Expected only one page that contains key:'0451'") diff --git a/scripts/fix.sh b/scripts/fix.sh index 8a74a46e8..6b933c988 100755 --- a/scripts/fix.sh +++ b/scripts/fix.sh @@ -10,3 +10,4 @@ XTESTGOFILES=$(${GO_CMD} list --f "{{with \$d:=.}}{{range .XTestGoFiles}}{{\$d. echo "${GOFILES}" "${TESTGOFILES}" "${XTESTGOFILES}"| xargs -n 100 go run golang.org/x/tools/cmd/goimports@latest -w -local go.etcd.io go fmt ./... +go mod tidy diff --git a/tx_test.go b/tx_test.go index 9d2f0422d..dca40fb54 100644 --- a/tx_test.go +++ b/tx_test.go @@ -48,8 +48,7 @@ func TestTx_Check_ReadOnly(t *testing.T) { numChecks := 2 errc := make(chan error, numChecks) check := func() { - err, _ := <-tx.Check(bolt.HexKeyValueStringer()) - errc <- err + errc <- <-tx.Check(bolt.HexKeyValueStringer()) } // Ensure the freelist is not reloaded and does not race. for i := 0; i < numChecks; i++ { From f16e2522ceec66d00517b7735baad2070fc0e685 Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Thu, 12 Jan 2023 17:39:04 +0100 Subject: [PATCH 5/7] Address review comments. Signed-off-by: Piotr Tabor --- cmd/bbolt/main.go | 39 +++++++++++----------- db.go | 2 +- db_test.go | 4 +-- internal/btesting/btesting.go | 2 +- internal/tests/tx_check_test.go | 53 +++++++++++++++--------------- tx.go | 2 +- tx_check.go | 57 +++++++++++++++++---------------- tx_test.go | 2 +- 8 files changed, 80 insertions(+), 81 deletions(-) diff --git a/cmd/bbolt/main.go b/cmd/bbolt/main.go index 8bca086db..a5be8b58c 100644 --- a/cmd/bbolt/main.go +++ b/cmd/bbolt/main.go @@ -207,7 +207,7 @@ func (cmd *CheckCommand) Run(args ...string) error { // Perform consistency check. return db.View(func(tx *bolt.Tx) error { var count int - for err := range tx.Check(CmdKeyValueStringer()) { + for err := range tx.Check(CmdKvStringer()) { fmt.Fprintln(cmd.Stdout, err) count++ } @@ -540,11 +540,7 @@ func formatBytes(b []byte, format string) (string, error) { case "bytes": return string(b), nil case "auto": - if isPrintable(string(b)) { - return string(b), nil - } else { - return fmt.Sprintf("%x", b), nil - } + return bytesToAsciiOrHex(b), nil case "redacted": return fmt.Sprintf("", len(b)), nil default: @@ -1573,6 +1569,15 @@ func isPrintable(s string) bool { return true } +func bytesToAsciiOrHex(b []byte) string { + sb := string(b) + if isPrintable(sb) { + return sb + } else { + return hex.EncodeToString(b) + } +} + func stringToPage(str string) (uint64, error) { return strconv.ParseUint(str, 10, 64) } @@ -1690,24 +1695,16 @@ Additional options include: `, "\n") } -type cmdKeyValueStringer struct{} +type cmdKvStringer struct{} -func (_ cmdKeyValueStringer) KeyToString(key []byte) string { - if isPrintable(string(key)) { - return string(key) - } else { - return hex.EncodeToString(key) - } +func (_ cmdKvStringer) KeyToString(key []byte) string { + return bytesToAsciiOrHex(key) } -func (_ cmdKeyValueStringer) ValueToString(value []byte) string { - if isPrintable(string(value)) { - return string(value) - } else { - return hex.EncodeToString(value) - } +func (_ cmdKvStringer) ValueToString(value []byte) string { + return bytesToAsciiOrHex(value) } -func CmdKeyValueStringer() bolt.KeyValueStringer { - return cmdKeyValueStringer{} +func CmdKvStringer() bolt.KVStringer { + return cmdKvStringer{} } diff --git a/db.go b/db.go index ed3c88681..5d0a029c2 100644 --- a/db.go +++ b/db.go @@ -1148,7 +1148,7 @@ func (db *DB) freepages() []pgid { panic(fmt.Sprintf("freepages: failed to get all reachable pages (%v)", e)) } }() - tx.checkBucket(&tx.root, reachable, nofreed, HexKeyValueStringer(), ech) + tx.checkBucket(&tx.root, reachable, nofreed, HexKVStringer(), ech) close(ech) // TODO: If check bucket reported any corruptions (ech) we shouldn't proceed to freeing the pages. diff --git a/db_test.go b/db_test.go index c7ce9cee0..5302ef6b1 100644 --- a/db_test.go +++ b/db_test.go @@ -396,7 +396,7 @@ func TestOpen_Check(t *testing.T) { if err != nil { t.Fatal(err) } - if err = db.View(func(tx *bolt.Tx) error { return <-tx.Check(bolt.HexKeyValueStringer()) }); err != nil { + if err = db.View(func(tx *bolt.Tx) error { return <-tx.Check(bolt.HexKVStringer()) }); err != nil { t.Fatal(err) } if err = db.Close(); err != nil { @@ -407,7 +407,7 @@ func TestOpen_Check(t *testing.T) { if err != nil { t.Fatal(err) } - if err := db.View(func(tx *bolt.Tx) error { return <-tx.Check(bolt.HexKeyValueStringer()) }); err != nil { + if err := db.View(func(tx *bolt.Tx) error { return <-tx.Check(bolt.HexKVStringer()) }); err != nil { t.Fatal(err) } if err := db.Close(); err != nil { diff --git a/internal/btesting/btesting.go b/internal/btesting/btesting.go index 42ec2b14f..1dc05a8ef 100644 --- a/internal/btesting/btesting.go +++ b/internal/btesting/btesting.go @@ -119,7 +119,7 @@ func (db *DB) MustCheck() { err := db.Update(func(tx *bolt.Tx) error { // Collect all the errors. var errors []error - for err := range tx.Check(bolt.HexKeyValueStringer()) { + for err := range tx.Check(bolt.HexKVStringer()) { errors = append(errors, err) if len(errors) > 10 { break diff --git a/internal/tests/tx_check_test.go b/internal/tests/tx_check_test.go index 7e9cc286a..336c98d1f 100644 --- a/internal/tests/tx_check_test.go +++ b/internal/tests/tx_check_test.go @@ -2,10 +2,9 @@ package tests_test import ( "fmt" + "github.com/stretchr/testify/require" "testing" - "github.com/stretchr/testify/assert" - bolt "go.etcd.io/bbolt" "go.etcd.io/bbolt/internal/btesting" "go.etcd.io/bbolt/internal/guts_cli" @@ -14,74 +13,74 @@ import ( func TestTx_RecursivelyCheckPages_MisplacedPage(t *testing.T) { db := btesting.MustCreateDB(t) - assert.NoError(t, + require.NoError(t, db.Fill([]byte("data"), 1, 10000, func(tx int, k int) []byte { return []byte(fmt.Sprintf("%04d", k)) }, func(tx int, k int) []byte { return make([]byte, 100) }, )) - assert.NoError(t, db.Close()) + require.NoError(t, db.Close()) xRay := surgeon.NewXRay(db.Path()) path1, err := xRay.FindPathsToKey([]byte("0451")) - assert.NoError(t, err, "Cannot find page that contains key:'0451'") - assert.Len(t, path1, 1, "Expected only one page that contains key:'0451'") + require.NoError(t, err, "Cannot find page that contains key:'0451'") + require.Len(t, path1, 1, "Expected only one page that contains key:'0451'") path2, err := xRay.FindPathsToKey([]byte("7563")) - assert.NoError(t, err, "Cannot find page that contains key:'7563'") - assert.Len(t, path2, 1, "Expected only one page that contains key:'7563'") + require.NoError(t, err, "Cannot find page that contains key:'7563'") + require.Len(t, path2, 1, "Expected only one page that contains key:'7563'") srcPage := path1[0][len(path1[0])-1] targetPage := path2[0][len(path2[0])-1] - assert.NoError(t, surgeon.CopyPage(db.Path(), srcPage, targetPage)) + require.NoError(t, surgeon.CopyPage(db.Path(), srcPage, targetPage)) db.MustReopen() - assert.NoError(t, db.Update(func(tx *bolt.Tx) error { + require.NoError(t, db.Update(func(tx *bolt.Tx) error { // Collect all the errors. var errors []error - for err := range tx.Check(bolt.HexKeyValueStringer()) { + for err := range tx.Check(bolt.HexKVStringer()) { errors = append(errors, err) } - assert.Len(t, errors, 1) - assert.ErrorContains(t, errors[0], fmt.Sprintf("leaf page(%v) needs to be >= to the key in the ancestor", targetPage)) + require.Len(t, errors, 1) + require.ErrorContains(t, errors[0], fmt.Sprintf("leaf page(%v) needs to be >= the key in the ancestor", targetPage)) return nil })) - assert.NoError(t, db.Close()) + require.NoError(t, db.Close()) } func TestTx_RecursivelyCheckPages_CorruptedLeaf(t *testing.T) { db := btesting.MustCreateDB(t) - assert.NoError(t, + require.NoError(t, db.Fill([]byte("data"), 1, 10000, func(tx int, k int) []byte { return []byte(fmt.Sprintf("%04d", k)) }, func(tx int, k int) []byte { return make([]byte, 100) }, )) - assert.NoError(t, db.Close()) + require.NoError(t, db.Close()) xray := surgeon.NewXRay(db.Path()) path1, err := xray.FindPathsToKey([]byte("0451")) - assert.NoError(t, err, "Cannot find page that contains key:'0451'") - assert.Len(t, path1, 1, "Expected only one page that contains key:'0451'") + require.NoError(t, err, "Cannot find page that contains key:'0451'") + require.Len(t, path1, 1, "Expected only one page that contains key:'0451'") srcPage := path1[0][len(path1[0])-1] p, pbuf, err := guts_cli.ReadPage(db.Path(), uint64(srcPage)) - assert.NoError(t, err) - assert.Positive(t, p.Count(), "page must be not empty") + require.NoError(t, err) + require.Positive(t, p.Count(), "page must be not empty") p.LeafPageElement(p.Count() / 2).Key()[0] = 'z' - assert.NoError(t, surgeon.WritePage(db.Path(), pbuf)) + require.NoError(t, surgeon.WritePage(db.Path(), pbuf)) db.MustReopen() - assert.NoError(t, db.Update(func(tx *bolt.Tx) error { + require.NoError(t, db.Update(func(tx *bolt.Tx) error { // Collect all the errors. var errors []error - for err := range tx.Check(bolt.HexKeyValueStringer()) { + for err := range tx.Check(bolt.HexKVStringer()) { errors = append(errors, err) } - assert.Len(t, errors, 2) - assert.ErrorContains(t, errors[0], fmt.Sprintf("leaf page(%v) needs to be < than key of the next element in ancestor", srcPage)) - assert.ErrorContains(t, errors[1], fmt.Sprintf("leaf page(%v) needs to be > (found <) than previous element", srcPage)) + require.Len(t, errors, 2) + require.ErrorContains(t, errors[0], fmt.Sprintf("leaf page(%v) needs to be < than key of the next element in ancestor", srcPage)) + require.ErrorContains(t, errors[1], fmt.Sprintf("leaf page(%v) needs to be > (found <) than previous element", srcPage)) return nil })) - assert.NoError(t, db.Close()) + require.NoError(t, db.Close()) } diff --git a/tx.go b/tx.go index e9f13b55d..c85027d91 100644 --- a/tx.go +++ b/tx.go @@ -190,7 +190,7 @@ func (tx *Tx) Commit() error { // If strict mode is enabled then perform a consistency check. if tx.db.StrictMode { - ch := tx.Check(HexKeyValueStringer()) + ch := tx.Check(HexKVStringer()) var errs []string for { err, ok := <-ch diff --git a/tx_check.go b/tx_check.go index 350cbf278..86984001d 100644 --- a/tx_check.go +++ b/tx_check.go @@ -13,13 +13,13 @@ import ( // because of caching. This overhead can be removed if running on a read-only // transaction, however, it is not safe to execute other writer transactions at // the same time. -func (tx *Tx) Check(keyValueStringer KeyValueStringer) <-chan error { +func (tx *Tx) Check(kvStringer KVStringer) <-chan error { ch := make(chan error) - go tx.check(keyValueStringer, ch) + go tx.check(kvStringer, ch) return ch } -func (tx *Tx) check(keyValueStringer KeyValueStringer, ch chan error) { +func (tx *Tx) check(kvStringer KVStringer, ch chan error) { // Force loading free list if opened in ReadOnly mode. tx.db.loadFreelist() @@ -45,7 +45,7 @@ func (tx *Tx) check(keyValueStringer KeyValueStringer, ch chan error) { } // Recursively check buckets. - tx.checkBucket(&tx.root, reachable, freed, keyValueStringer, ch) + tx.checkBucket(&tx.root, reachable, freed, kvStringer, ch) // Ensure all pages below high water mark are either reachable or freed. for i := pgid(0); i < tx.meta.pgid; i++ { @@ -60,7 +60,7 @@ func (tx *Tx) check(keyValueStringer KeyValueStringer, ch chan error) { } func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bool, - keyValueStringer KeyValueStringer, ch chan error) { + kvStringer KVStringer, ch chan error) { // Ignore inline buckets. if b.root == 0 { return @@ -89,12 +89,12 @@ func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bo } }) - tx.recursivelyCheckPages(b.root, keyValueStringer.KeyToString, ch) + tx.recursivelyCheckPages(b.root, kvStringer.KeyToString, ch) // Check each bucket within this bucket. _ = b.ForEachBucket(func(k []byte) error { if child := b.Bucket(k); child != nil { - tx.checkBucket(child, reachable, freed, keyValueStringer, ch) + tx.checkBucket(child, reachable, freed, kvStringer, ch) } return nil }) @@ -127,8 +127,8 @@ func (tx *Tx) recursivelyCheckPagesInternal( elem := p.branchPageElement(uint16(i)) if i == 0 && runningMin != nil && compareKeys(runningMin, elem.key()) > 0 { ch <- fmt.Errorf("key (%d, %s) on the branch page(%d) needs to be >="+ - " to the index in the ancestor. Pages stack: %v", - i, keyToString(elem.key()), pgid, pagesStack) + " to the key(%s) in the ancestor. Pages stack: %v", + i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) } if maxKeyOpen != nil && compareKeys(elem.key(), maxKeyOpen) >= 0 { @@ -146,22 +146,25 @@ func (tx *Tx) recursivelyCheckPagesInternal( maxKeyInSubtree = tx.recursivelyCheckPagesInternal(elem.pgid, elem.key(), maxKey, pagesStack, keyToString, ch) runningMin = maxKeyInSubtree } - return + return maxKeyInSubtree case p.flags&leafPageFlag != 0: runningMin := minKeyClosed for i := range p.leafPageElements() { elem := p.leafPageElement(uint16(i)) if i == 0 && runningMin != nil && compareKeys(runningMin, elem.key()) > 0 { - ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be >= to the key in the ancestor. Stack: %v", - i, keyToString(elem.key()), pgid, pagesStack) - } - if i > 0 && compareKeys(runningMin, elem.key()) > 0 { - ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found <) than previous element (hex)%s. Stack: %v", + ch <- fmt.Errorf("The first key[%d]=(hex)%s on leaf page(%d) needs to be >= the key in the ancestor (%s). Stack: %v", i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) } - if i > 0 && compareKeys(runningMin, elem.key()) == 0 { - ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found =) than previous element (hex)%s. Stack: %v", - i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) + if i > 0 { + cmpRet := compareKeys(runningMin, elem.key()) + if cmpRet > 0 { + ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found <) than previous element (hex)%s. Stack: %v", + i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) + } + if cmpRet == 0 { + ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found =) than previous element (hex)%s. Stack: %v", + i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) + } } if maxKeyOpen != nil && compareKeys(elem.key(), maxKeyOpen) >= 0 { ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be < than key of the next element in ancestor (hex)%s. Pages stack: %v", @@ -175,28 +178,28 @@ func (tx *Tx) recursivelyCheckPagesInternal( default: ch <- fmt.Errorf("unexpected page type for pgid:%d", pgid) } - return nil + return maxKeyInSubtree } // =========================================================================================== -// KeyValueStringer allows to prepare human-readable diagnostic messages. -type KeyValueStringer interface { +// KVStringer allows to prepare human-readable diagnostic messages. +type KVStringer interface { KeyToString([]byte) string ValueToString([]byte) string } -// HexKeyValueStringer serializes both key & value to hex representation. -func HexKeyValueStringer() KeyValueStringer { - return hexKeyValueStringer{} +// HexKVStringer serializes both key & value to hex representation. +func HexKVStringer() KVStringer { + return hexKvStringer{} } -type hexKeyValueStringer struct{} +type hexKvStringer struct{} -func (_ hexKeyValueStringer) KeyToString(key []byte) string { +func (_ hexKvStringer) KeyToString(key []byte) string { return hex.EncodeToString(key) } -func (_ hexKeyValueStringer) ValueToString(value []byte) string { +func (_ hexKvStringer) ValueToString(value []byte) string { return hex.EncodeToString(value) } diff --git a/tx_test.go b/tx_test.go index dca40fb54..5b74f04db 100644 --- a/tx_test.go +++ b/tx_test.go @@ -48,7 +48,7 @@ func TestTx_Check_ReadOnly(t *testing.T) { numChecks := 2 errc := make(chan error, numChecks) check := func() { - errc <- <-tx.Check(bolt.HexKeyValueStringer()) + errc <- <-tx.Check(bolt.HexKVStringer()) } // Ensure the freelist is not reloaded and does not race. for i := 0; i < numChecks; i++ { From 80edaf14f0bd76ff85d0346c69ac9fbe07fb5473 Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Fri, 13 Jan 2023 17:40:27 +0100 Subject: [PATCH 6/7] Rename: pgid pgid => pgId pgid to avoid confusion. Signed-off-by: Piotr Tabor --- bucket.go | 12 ++++++------ cursor.go | 20 ++++++++++---------- freelist.go | 4 ++-- node.go | 8 ++++---- tx_check.go | 24 ++++++++++++------------ 5 files changed, 34 insertions(+), 34 deletions(-) diff --git a/bucket.go b/bucket.go index 5ec9da738..054467af3 100644 --- a/bucket.go +++ b/bucket.go @@ -513,8 +513,8 @@ func (b *Bucket) forEachPageNode(fn func(*page, *node, int)) { b._forEachPageNode(b.root, 0, fn) } -func (b *Bucket) _forEachPageNode(pgid pgid, depth int, fn func(*page, *node, int)) { - var p, n = b.pageNode(pgid) +func (b *Bucket) _forEachPageNode(pgId pgid, depth int, fn func(*page, *node, int)) { + var p, n = b.pageNode(pgId) // Execute function. fn(p, n, depth) @@ -654,11 +654,11 @@ func (b *Bucket) rebalance() { } // node creates a node from a page and associates it with a given parent. -func (b *Bucket) node(pgid pgid, parent *node) *node { +func (b *Bucket) node(pgId pgid, parent *node) *node { _assert(b.nodes != nil, "nodes map expected") // Retrieve node if it's already been created. - if n := b.nodes[pgid]; n != nil { + if n := b.nodes[pgId]; n != nil { return n } @@ -673,12 +673,12 @@ func (b *Bucket) node(pgid pgid, parent *node) *node { // Use the inline page if this is an inline bucket. var p = b.page if p == nil { - p = b.tx.page(pgid) + p = b.tx.page(pgId) } // Read the page into the node and cache it. n.read(p) - b.nodes[pgid] = n + b.nodes[pgId] = n // Update statistics. b.tx.stats.IncNodeCount(1) diff --git a/cursor.go b/cursor.go index a6279f7ca..5dafb0cac 100644 --- a/cursor.go +++ b/cursor.go @@ -172,13 +172,13 @@ func (c *Cursor) goToFirstElementOnTheStack() { } // Keep adding pages pointing to the first element to the stack. - var pgid pgid + var pgId pgid if ref.node != nil { - pgid = ref.node.inodes[ref.index].pgid + pgId = ref.node.inodes[ref.index].pgid } else { - pgid = ref.page.branchPageElement(uint16(ref.index)).pgid + pgId = ref.page.branchPageElement(uint16(ref.index)).pgid } - p, n := c.bucket.pageNode(pgid) + p, n := c.bucket.pageNode(pgId) c.stack = append(c.stack, elemRef{page: p, node: n, index: 0}) } } @@ -193,13 +193,13 @@ func (c *Cursor) last() { } // Keep adding pages pointing to the last element in the stack. - var pgid pgid + var pgId pgid if ref.node != nil { - pgid = ref.node.inodes[ref.index].pgid + pgId = ref.node.inodes[ref.index].pgid } else { - pgid = ref.page.branchPageElement(uint16(ref.index)).pgid + pgId = ref.page.branchPageElement(uint16(ref.index)).pgid } - p, n := c.bucket.pageNode(pgid) + p, n := c.bucket.pageNode(pgId) var nextRef = elemRef{page: p, node: n} nextRef.index = nextRef.count() - 1 @@ -268,8 +268,8 @@ func (c *Cursor) prev() (key []byte, value []byte, flags uint32) { } // search recursively performs a binary search against a given page/node until it finds a given key. -func (c *Cursor) search(key []byte, pgid pgid) { - p, n := c.bucket.pageNode(pgid) +func (c *Cursor) search(key []byte, pgId pgid) { + p, n := c.bucket.pageNode(pgId) if p != nil && (p.flags&(branchPageFlag|leafPageFlag)) == 0 { panic(fmt.Sprintf("invalid page type: %d: %x", p.id, p.flags)) } diff --git a/freelist.go b/freelist.go index df1ae6485..50f2d0e17 100644 --- a/freelist.go +++ b/freelist.go @@ -256,8 +256,8 @@ func (f *freelist) rollback(txid txid) { } // freed returns whether a given page is in the free list. -func (f *freelist) freed(pgid pgid) bool { - _, ok := f.cache[pgid] +func (f *freelist) freed(pgId pgid) bool { + _, ok := f.cache[pgId] return ok } diff --git a/node.go b/node.go index 321536bd9..9c56150d8 100644 --- a/node.go +++ b/node.go @@ -113,9 +113,9 @@ func (n *node) prevSibling() *node { } // put inserts a key/value. -func (n *node) put(oldKey, newKey, value []byte, pgid pgid, flags uint32) { - if pgid >= n.bucket.tx.meta.pgid { - panic(fmt.Sprintf("pgid (%d) above high water mark (%d)", pgid, n.bucket.tx.meta.pgid)) +func (n *node) put(oldKey, newKey, value []byte, pgId pgid, flags uint32) { + if pgId >= n.bucket.tx.meta.pgid { + panic(fmt.Sprintf("pgId (%d) above high water mark (%d)", pgId, n.bucket.tx.meta.pgid)) } else if len(oldKey) <= 0 { panic("put: zero-length old key") } else if len(newKey) <= 0 { @@ -136,7 +136,7 @@ func (n *node) put(oldKey, newKey, value []byte, pgid pgid, flags uint32) { inode.flags = flags inode.key = newKey inode.value = value - inode.pgid = pgid + inode.pgid = pgId _assert(len(inode.key) > 0, "put: zero-length inode key") } diff --git a/tx_check.go b/tx_check.go index 86984001d..0c8299764 100644 --- a/tx_check.go +++ b/tx_check.go @@ -104,8 +104,8 @@ func (tx *Tx) checkBucket(b *Bucket, reachable map[pgid]*page, freed map[pgid]bo // key order constraints: // - keys on pages must be sorted // - keys on children pages are between 2 consecutive keys on the parent's branch page). -func (tx *Tx) recursivelyCheckPages(pgid pgid, keyToString func([]byte) string, ch chan error) { - tx.recursivelyCheckPagesInternal(pgid, nil, nil, nil, keyToString, ch) +func (tx *Tx) recursivelyCheckPages(pgId pgid, keyToString func([]byte) string, ch chan error) { + tx.recursivelyCheckPagesInternal(pgId, nil, nil, nil, keyToString, ch) } // recursivelyCheckPagesInternal verifies that all keys in the subtree rooted at `pgid` are: @@ -114,11 +114,11 @@ func (tx *Tx) recursivelyCheckPages(pgid pgid, keyToString func([]byte) string, // - Are in right ordering relationship to their parents. // `pagesStack` is expected to contain IDs of pages from the tree root to `pgid` for the clean debugging message. func (tx *Tx) recursivelyCheckPagesInternal( - pgid pgid, minKeyClosed, maxKeyOpen []byte, pagesStack []pgid, + pgId pgid, minKeyClosed, maxKeyOpen []byte, pagesStack []pgid, keyToString func([]byte) string, ch chan error) (maxKeyInSubtree []byte) { - p := tx.page(pgid) - pagesStack = append(pagesStack, pgid) + p := tx.page(pgId) + pagesStack = append(pagesStack, pgId) switch { case p.flags&branchPageFlag != 0: // For branch page we navigate ranges of all subpages. @@ -128,13 +128,13 @@ func (tx *Tx) recursivelyCheckPagesInternal( if i == 0 && runningMin != nil && compareKeys(runningMin, elem.key()) > 0 { ch <- fmt.Errorf("key (%d, %s) on the branch page(%d) needs to be >="+ " to the key(%s) in the ancestor. Pages stack: %v", - i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) + i, keyToString(elem.key()), pgId, keyToString(runningMin), pagesStack) } if maxKeyOpen != nil && compareKeys(elem.key(), maxKeyOpen) >= 0 { ch <- fmt.Errorf("key (%d: %s) on the branch page(%d) needs to be <"+ " than key of the next element reachable from the ancestor (%v). Pages stack: %v", - i, keyToString(elem.key()), pgid, keyToString(maxKeyOpen), pagesStack) + i, keyToString(elem.key()), pgId, keyToString(maxKeyOpen), pagesStack) } var maxKey []byte @@ -153,22 +153,22 @@ func (tx *Tx) recursivelyCheckPagesInternal( elem := p.leafPageElement(uint16(i)) if i == 0 && runningMin != nil && compareKeys(runningMin, elem.key()) > 0 { ch <- fmt.Errorf("The first key[%d]=(hex)%s on leaf page(%d) needs to be >= the key in the ancestor (%s). Stack: %v", - i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) + i, keyToString(elem.key()), pgId, keyToString(runningMin), pagesStack) } if i > 0 { cmpRet := compareKeys(runningMin, elem.key()) if cmpRet > 0 { ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found <) than previous element (hex)%s. Stack: %v", - i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) + i, keyToString(elem.key()), pgId, keyToString(runningMin), pagesStack) } if cmpRet == 0 { ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found =) than previous element (hex)%s. Stack: %v", - i, keyToString(elem.key()), pgid, keyToString(runningMin), pagesStack) + i, keyToString(elem.key()), pgId, keyToString(runningMin), pagesStack) } } if maxKeyOpen != nil && compareKeys(elem.key(), maxKeyOpen) >= 0 { ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be < than key of the next element in ancestor (hex)%s. Pages stack: %v", - i, keyToString(elem.key()), pgid, keyToString(maxKeyOpen), pagesStack) + i, keyToString(elem.key()), pgId, keyToString(maxKeyOpen), pagesStack) } runningMin = elem.key() } @@ -176,7 +176,7 @@ func (tx *Tx) recursivelyCheckPagesInternal( return p.leafPageElement(p.count - 1).key() } default: - ch <- fmt.Errorf("unexpected page type for pgid:%d", pgid) + ch <- fmt.Errorf("unexpected page type for pgId:%d", pgId) } return maxKeyInSubtree } From eb0deb95501f37d8a800e9e8d3318dc284635e50 Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Fri, 13 Jan 2023 19:13:16 +0100 Subject: [PATCH 7/7] Refactor common code within key-order checker. Signed-off-by: Piotr Tabor --- internal/tests/tx_check_test.go | 9 ++--- tx_check.go | 62 +++++++++++++++------------------ 2 files changed, 34 insertions(+), 37 deletions(-) diff --git a/internal/tests/tx_check_test.go b/internal/tests/tx_check_test.go index 336c98d1f..342198fa5 100644 --- a/internal/tests/tx_check_test.go +++ b/internal/tests/tx_check_test.go @@ -2,9 +2,10 @@ package tests_test import ( "fmt" - "github.com/stretchr/testify/require" "testing" + "github.com/stretchr/testify/require" + bolt "go.etcd.io/bbolt" "go.etcd.io/bbolt/internal/btesting" "go.etcd.io/bbolt/internal/guts_cli" @@ -23,11 +24,11 @@ func TestTx_RecursivelyCheckPages_MisplacedPage(t *testing.T) { xRay := surgeon.NewXRay(db.Path()) path1, err := xRay.FindPathsToKey([]byte("0451")) - require.NoError(t, err, "Cannot find page that contains key:'0451'") + require.NoError(t, err, "cannot find page that contains key:'0451'") require.Len(t, path1, 1, "Expected only one page that contains key:'0451'") path2, err := xRay.FindPathsToKey([]byte("7563")) - require.NoError(t, err, "Cannot find page that contains key:'7563'") + require.NoError(t, err, "cannot find page that contains key:'7563'") require.Len(t, path2, 1, "Expected only one page that contains key:'7563'") srcPage := path1[0][len(path1[0])-1] @@ -60,7 +61,7 @@ func TestTx_RecursivelyCheckPages_CorruptedLeaf(t *testing.T) { xray := surgeon.NewXRay(db.Path()) path1, err := xray.FindPathsToKey([]byte("0451")) - require.NoError(t, err, "Cannot find page that contains key:'0451'") + require.NoError(t, err, "cannot find page that contains key:'0451'") require.Len(t, path1, 1, "Expected only one page that contains key:'0451'") srcPage := path1[0][len(path1[0])-1] diff --git a/tx_check.go b/tx_check.go index 0c8299764..23e22c3da 100644 --- a/tx_check.go +++ b/tx_check.go @@ -125,23 +125,11 @@ func (tx *Tx) recursivelyCheckPagesInternal( runningMin := minKeyClosed for i := range p.branchPageElements() { elem := p.branchPageElement(uint16(i)) - if i == 0 && runningMin != nil && compareKeys(runningMin, elem.key()) > 0 { - ch <- fmt.Errorf("key (%d, %s) on the branch page(%d) needs to be >="+ - " to the key(%s) in the ancestor. Pages stack: %v", - i, keyToString(elem.key()), pgId, keyToString(runningMin), pagesStack) - } - - if maxKeyOpen != nil && compareKeys(elem.key(), maxKeyOpen) >= 0 { - ch <- fmt.Errorf("key (%d: %s) on the branch page(%d) needs to be <"+ - " than key of the next element reachable from the ancestor (%v). Pages stack: %v", - i, keyToString(elem.key()), pgId, keyToString(maxKeyOpen), pagesStack) - } + verifyKeyOrder(elem.pgid, "branch", i, elem.key(), runningMin, maxKeyOpen, ch, keyToString, pagesStack) - var maxKey []byte + maxKey := maxKeyOpen if i < len(p.branchPageElements())-1 { maxKey = p.branchPageElement(uint16(i + 1)).key() - } else { - maxKey = maxKeyOpen } maxKeyInSubtree = tx.recursivelyCheckPagesInternal(elem.pgid, elem.key(), maxKey, pagesStack, keyToString, ch) runningMin = maxKeyInSubtree @@ -151,25 +139,7 @@ func (tx *Tx) recursivelyCheckPagesInternal( runningMin := minKeyClosed for i := range p.leafPageElements() { elem := p.leafPageElement(uint16(i)) - if i == 0 && runningMin != nil && compareKeys(runningMin, elem.key()) > 0 { - ch <- fmt.Errorf("The first key[%d]=(hex)%s on leaf page(%d) needs to be >= the key in the ancestor (%s). Stack: %v", - i, keyToString(elem.key()), pgId, keyToString(runningMin), pagesStack) - } - if i > 0 { - cmpRet := compareKeys(runningMin, elem.key()) - if cmpRet > 0 { - ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found <) than previous element (hex)%s. Stack: %v", - i, keyToString(elem.key()), pgId, keyToString(runningMin), pagesStack) - } - if cmpRet == 0 { - ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be > (found =) than previous element (hex)%s. Stack: %v", - i, keyToString(elem.key()), pgId, keyToString(runningMin), pagesStack) - } - } - if maxKeyOpen != nil && compareKeys(elem.key(), maxKeyOpen) >= 0 { - ch <- fmt.Errorf("key[%d]=(hex)%s on leaf page(%d) needs to be < than key of the next element in ancestor (hex)%s. Pages stack: %v", - i, keyToString(elem.key()), pgId, keyToString(maxKeyOpen), pagesStack) - } + verifyKeyOrder(pgId, "leaf", i, elem.key(), runningMin, maxKeyOpen, ch, keyToString, pagesStack) runningMin = elem.key() } if p.count > 0 { @@ -181,6 +151,32 @@ func (tx *Tx) recursivelyCheckPagesInternal( return maxKeyInSubtree } +/*** + * verifyKeyOrder checks whether an entry with given #index on pgId (pageType: "branch|leaf") that has given "key", + * is within range determined by (previousKey..maxKeyOpen) and reports found violations to the channel (ch). + */ +func verifyKeyOrder(pgId pgid, pageType string, index int, key []byte, previousKey []byte, maxKeyOpen []byte, ch chan error, keyToString func([]byte) string, pagesStack []pgid) { + if index == 0 && previousKey != nil && compareKeys(previousKey, key) > 0 { + ch <- fmt.Errorf("the first key[%d]=(hex)%s on %s page(%d) needs to be >= the key in the ancestor (%s). Stack: %v", + index, keyToString(key), pageType, pgId, keyToString(previousKey), pagesStack) + } + if index > 0 { + cmpRet := compareKeys(previousKey, key) + if cmpRet > 0 { + ch <- fmt.Errorf("key[%d]=(hex)%s on %s page(%d) needs to be > (found <) than previous element (hex)%s. Stack: %v", + index, keyToString(key), pageType, pgId, keyToString(previousKey), pagesStack) + } + if cmpRet == 0 { + ch <- fmt.Errorf("key[%d]=(hex)%s on %s page(%d) needs to be > (found =) than previous element (hex)%s. Stack: %v", + index, keyToString(key), pageType, pgId, keyToString(previousKey), pagesStack) + } + } + if maxKeyOpen != nil && compareKeys(key, maxKeyOpen) >= 0 { + ch <- fmt.Errorf("key[%d]=(hex)%s on %s page(%d) needs to be < than key of the next element in ancestor (hex)%s. Pages stack: %v", + index, keyToString(key), pageType, pgId, keyToString(previousKey), pagesStack) + } +} + // =========================================================================================== // KVStringer allows to prepare human-readable diagnostic messages.