-
Notifications
You must be signed in to change notification settings - Fork 628
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
add option to skip freelist sync #1
Conversation
236381f
to
dacec63
Compare
bucket_test.go
Outdated
@@ -13,7 +13,7 @@ import ( | |||
"testing" | |||
"testing/quick" | |||
|
|||
"github.com/boltdb/bolt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this renaming be split into a separate patch to merge first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. will do.
@heyitsanthony fixed. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks OK, but is switching between no free list sync and free list sync safe? Not super worried about leaking pages, but loading a stale free list and scribbling over an allocated page would be bad
db.go
Outdated
return fids | ||
} | ||
|
||
func checkReachable(tx *Tx, b *Bucket, reachable map[pgid]*page) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is very similar to Tx.checkBucket
; would it be possible to have this so both share some code? Maybe func (tx *Tx) freepages() []pgid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will give it a try.
db_test.go
Outdated
} | ||
|
||
// MustOpenDB returns a new, open DB at a temporary location with given options. | ||
func MustOpenWitOption(o *bolt.Options) *DB { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Wit/With
db_test.go
Outdated
} | ||
} | ||
|
||
// MustOpenDB returns a new, open DB at a temporary location with given options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// MustOpenWithOpen
...
db_test.go
Outdated
@@ -1399,6 +1419,15 @@ func (db *DB) MustClose() { | |||
} | |||
} | |||
|
|||
// MustClose reopen the database. Panic on error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// MustReopen reopens
...
freelist.go
Outdated
@@ -22,6 +22,14 @@ func newFreelist() *freelist { | |||
} | |||
} | |||
|
|||
func newFreelistWithIDs(ids []pgid) *freelist { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some indexing assumptions might be violated by passing in a raw pgid slice, how about:
func (f *freelist) readIds(ids []pgid) {
f.ids = ids
...
}
where freelist.read()
's tail calls into readIds()
tx.go
Outdated
if tx.meta.pgid > opgid { | ||
if err := tx.db.grow(int(tx.meta.pgid+1) * tx.db.pageSize); err != nil { | ||
if !tx.db.NoFreelistSync { | ||
opgid := tx.meta.pgid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split into a separate method like tx.commitFreelist(...)
?
it is safe from free list sync to no sync, but not the other way around. |
@heyitsanthony fixed. PTAL. |
lgtm but the test/second patch seems to have code that belongs in the implementation/first patch. Also it'd be nice to have some way to protect against no sync -> sync (commit with no-sync writes an invalid page for the freelist?), but that can be deferred; open an issue? |
yea... i made a mistake when rebasing... I guess i will just squash the two commits into one. I will open an issue on the other thing you mentioned. |
When the database has a lot of freepages, the cost to sync all freepages down to disk is high. If the total database size is small (<10GB), and the application can tolerate ~10 seconds recovery time, then it is reasonable to simply not sync freelist and rescan the db to rebuild freelist on recovery.
When the database has a lot of freepages, the cost to sync all
freepages down to disk is high. If the total database size is
small (<10GB), and the application can tolerate ~10 seconds
recovery time, then it is reasonable to simply not sync freelist
and rescan the db to rebuild freelist on recovery.