tx: load freelist on Check() #49
Conversation
heyitsanthony
force-pushed
the
fix-readonly-check
branch
from
26225a4
to
e21b34b
Compare
Otherwise, nil dereference on ReadOnly DB Fixes boltdb#45
heyitsanthony
force-pushed
the
fix-readonly-check
branch
from
e21b34b
to
6f3dcc3
Compare
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
=========================================
Coverage ? 85.59%
=========================================
Files ? 9
Lines ? 1860
Branches ? 0
=========================================
Hits ? 1592
Misses ? 159
Partials ? 109
Continue to review full report at Codecov.
|
|
added a little extra synchronization machinery to handle this, but it shouldn't impact the common path |
| @@ -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 | |||
| // concurrenct accesses are being made to the freelist. | |||
There was a problem hiding this comment.
"concurrent accesses being made to the freelist."
| db.stats.FreePageN = len(db.freelist.ids) | ||
| close(db.freelistReady) | ||
| }) | ||
| <-db.freelistReady |
There was a problem hiding this comment.
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
| @@ -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 | |||
There was a problem hiding this comment.
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...."
| @@ -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 | |||
| // concurrenct accesses are being made to the freelist. | |||
| func (db *DB) readFreelist() bool { | |||
There was a problem hiding this comment.
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.
|
We will be completing this PR via #52. |
|
closing in favor of #49 |
Fixes #45