Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tx: load freelist on Check() #49

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions db.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,12 @@ type DB struct {
opened bool
rwtx *Tx
txs []*Tx
freelist *freelist
stats Stats

freelist *freelist
freelistLoad sync.Once
freelistReady chan struct{} // closed once freelist is loaded

pagePool sync.Pool

batchMu sync.Mutex
Expand Down Expand Up @@ -157,8 +160,10 @@ func (db *DB) String() string {
// If the file does not exist then it will be created automatically.
// Passing in nil options will cause Bolt to open the database with the default options.
func Open(path string, mode os.FileMode, options *Options) (*DB, error) {
var db = &DB{opened: true}

db := &DB{
opened: true,
freelistReady: make(chan struct{}),
}
// Set default options if no options are provided.
if options == nil {
options = DefaultOptions
Expand Down Expand Up @@ -254,20 +259,11 @@ func Open(path string, mode os.FileMode, options *Options) (*DB, error) {
return db, nil
}

db.freelist = newFreelist()
noFreeList := db.meta().freelist == pgidNoFreelist
if noFreeList {
// Reconstruct free list by scanning the DB.
db.freelist.readIDs(db.freepages())
} else {
// Read free list from freelist page.
db.freelist.read(db.page(db.meta().freelist))
}
db.stats.FreePageN = len(db.freelist.ids)
hasFreeList := db.readFreelist()

// Flush freelist when transitioning from no sync to sync so
// NoFreelistSync unaware boltdb can open the db later.
if !db.NoFreelistSync && noFreeList {
if !db.NoFreelistSync && !hasFreeList {
tx, err := db.Begin(true)
if tx != nil {
err = tx.Commit()
Expand All @@ -282,6 +278,26 @@ func Open(path string, mode os.FileMode, options *Options) (*DB, error) {
return db, nil
}

// readFreelist is called on Open and tx.Check. It assumes there are no
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename this to loadFreelist? That might more strongly suggest what side effects it has.

Might also be worth summarizing purpose of the function in a bit more detail, e.g:

"loadFreelist reads the freelist if it is synced, or reconstructs it by scanning the DB if it is not synced. This must be called before any operations that require on the freelist are executed...."

// concurrenct accesses are being made to the freelist.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"concurrent accesses being made to the freelist."

func (db *DB) readFreelist() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the return value of this function a bit tricky to reason about.

It might be cleaner to not return bool and fully separate the concerns of loading the freelist in readFreelist from the concerns of flushing the freelist in db.Open. E.g.:

db.Open:

...
if db.readOnly {
	return db, nil
}
hasFreeList := db.meta().freelist != pgidNoFreelist
db.readFreelist()

if !db.NoFreelistSync && !hasFreeList {
...

db.readFreelist:

func (db *DB) readFreelist() {
  b.freelistLoad.Do(func() {
		hasFreeList := db.meta().freelist != pgidNoFreelist
		db.freelist = newFreelist()
		if !hasFreeList {
...

And we could even add a db.hasSyncedFreelist() function to encapsulate the db.meta().freelist != pgidNoFreelist check and clean up the code a bit more.

hasFreeList := db.meta().freelist != pgidNoFreelist
db.freelistLoad.Do(func() {
db.freelist = newFreelist()
if !hasFreeList {
// Reconstruct free list by scanning the DB.
db.freelist.readIDs(db.freepages())
} else {
// Read free list from freelist page.
db.freelist.read(db.page(db.meta().freelist))
}
db.stats.FreePageN = len(db.freelist.ids)
close(db.freelistReady)
})
<-db.freelistReady
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conveniently, it looks like we don't need the freelistReady channel. Since "...no call to Do returns until the one call to f returns" (go sync.Once docs), we don't need to wait on the freelistReady channel, it is guaranteed to always be closed.

Go playground sanity check: https://play.golang.org/p/ZJEX1V-IbY

return hasFreeList
}

// mmap opens the underlying memory-mapped file and initializes the meta references.
// minsz is the minimum size that the new mmap can be.
func (db *DB) mmap(minsz int) error {
Expand Down
3 changes: 3 additions & 0 deletions tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,9 @@ func (tx *Tx) Check() <-chan error {
}

func (tx *Tx) check(ch chan error) {
// Force loading free list if opened in ReadOnly mode.
tx.db.readFreelist()

// Check if any pages are double freed.
freed := make(map[pgid]bool)
all := make([]pgid, tx.db.freelist.count())
Expand Down
48 changes: 48 additions & 0 deletions tx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,54 @@ import (
"github.com/coreos/bbolt"
)

// TestTx_Check_ReadOnly tests consistency checking on a ReadOnly database.
func TestTx_Check_ReadOnly(t *testing.T) {
db := MustOpenDB()
defer db.Close()
if err := db.Update(func(tx *bolt.Tx) error {
b, err := tx.CreateBucket([]byte("widgets"))
if err != nil {
t.Fatal(err)
}
if err := b.Put([]byte("foo"), []byte("bar")); err != nil {
t.Fatal(err)
}
return nil
}); err != nil {
t.Fatal(err)
}
if err := db.DB.Close(); err != nil {
t.Fatal(err)
}

readOnlyDB, err := bolt.Open(db.f, 0666, &bolt.Options{ReadOnly: true})
if err != nil {
t.Fatal(err)
}
defer readOnlyDB.Close()

tx, err := readOnlyDB.Begin(false)
if err != nil {
t.Fatal(err)
}
// ReadOnly DB will load freelist on Check call.
numChecks := 2
errc := make(chan error, numChecks)
check := func() {
err, _ := <-tx.Check()
errc <- err
}
// Ensure the freelist is not reloaded and does not race.
for i := 0; i < numChecks; i++ {
go check()
}
for i := 0; i < numChecks; i++ {
if err := <-errc; err != nil {
t.Fatal(err)
}
}
}

// Ensure that committing a closed transaction returns an error.
func TestTx_Commit_ErrTxClosed(t *testing.T) {
db := MustOpenDB()
Expand Down