scriptoriumadmin/featurebase
1
0
Fork 0
mirror of https://github.com/featurebasedb/featurebase.git synced 2026-10-09 12:27:52 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

There are two related bugs here.

Browse source 
First, it is possible for us to end up allocating *or freeing* pages during
a modification of the free list, in a way such that the change to the free list
means that when we finish the modification which caused the allocate or free,
we've overwritten the inner change.

Second, when deallocating trees, we don't actually deallocate the branch nodes
themselves.

The former causes potentially severe data corruption. The latter causes us
to gradually leak pages in a way that we don't notice because we only run those
tests during the RBF tests.

The fix for this is surprisingly intricate, because of the counterintuitive
fact that *allocating* a page means *removing* things from the free list
(and thus potentially deallocating free list pages), while *freeing* a page
means *adding* things to the free list (and thus potentially needing to
allocate pages for the free list).

While modifying the free list, any allocations we need always just come from
the end of the file; we don't try to reuse free pages. If a page becomes
*deallocated* by a free list modification, we don't annotate it in the free
list at the instant that it happens; we stash that information until the
current modification of the free list happens, then iterate through any
such pages.

I am pretty sure there's virtually never more than one, and I don't actually
know that I can create a case wherein we'd end up with the nested case
firing, wherein removing a page from the free list causes us to remove another
page, but I think if the free list got large and cluttered and needed
rebalancing or something it could maybe happen.
This commit is contained in:
Seebs 2021-11-10 17:27:12 -06:00
parent a38c219cf3
commit 9c0c0ec8c1
2 changed files with 171 additions and 11 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

93
rbf/tx.go
View file

@ -56,6 +56,27 @@ type Tx struct {
// behavior where an existing container has all its bits cleared
// but still sticks around in the database.
DeleteEmptyContainer bool
// It is possible for a modification of the free list to cause a page to
// be allocated or deallocated, which would modify the free list.
//
// For the case where pages need to be allocated during free list
// changes, we can trivially just allocate new pages and not use the
// free list. That's the simple case...
modifyingFreelist bool
// But removals can't be deferred/not-done like that. If a free list
// change causes us to deallocate a page (such as if we're *allocating*
// a page, which causes it to be *removed* from the free list), we really
// do need to record that, but if we try to do it during the update
// process, things could go horribly wrong. So, we have a transient list
// of page numbers which have been deallocated, but it happened *during*
// the modification of the free list. The top-level modification then
// processes them on its way out, using a defer. During the processing
// of this list, we *still* have the flag set, and we make a new list
// while processing the list, so if somehow a pending add to the list
// manages to trigger a *deallocation* (which I don't think should be
// happening), we'll process that one after the current list is processed.
pendingFreelistAdds []uint32
}
func (tx *Tx) DBPath() string {
@ -903,16 +924,67 @@ func (tx *Tx) walkTree(pgno, parent uint32, fn func(pgno, parent, typ uint32) er
}
}
// freelistCleanup handles things which we need to add to the free list,
// because they became free during the process of modifying the free list.
// It also marks us as done modifying the free list. Expected usage is that
// you set modifyingFreelist to true, then defer this.
//
// if you are modifying the free list, we can't further change the free
// list during that modification. for page allocations, we can just skip
// the free list check. for deallocations, though, we do need to mark them
// as freed at some point. so, if we're modifying the free list when
// a new freePgno happens, we stash the new pages in here, then apply
// them afterwards. so far as i know, this can actually only happen
// during an allocate, when we're removing entries from the free list, and
// the add path doesn't ever trigger it. so, when we remove entries from
// the free list, it's possible that doing so frees up pages that were
// part of the free list, and we then add them. but we don't have to worry
// about that removing things from the free list, because the add logic
// already just uses new pages rather than trying to use the free list
// when it knows the free list is involved.
func (tx *Tx) freelistCleanup(outErr *error) {
defer func() {
// no matter what, we're done with this after this, but we still
// want it set *while* we do this so nothing we do will have side
// effects that collide with what we're doing.
tx.modifyingFreelist = false
}()
if len(tx.pendingFreelistAdds) == 0 {
return
}
c := Cursor{tx: tx}
c.stack.elems[0] = stackElem{pgno: readMetaFreelistPageNo(tx.meta[:])}
for len(tx.pendingFreelistAdds) > 0 {
var pass []uint32
pass, tx.pendingFreelistAdds = tx.pendingFreelistAdds, nil
for _, pgno := range pass {
if changed, err := c.Add(uint64(pgno)); err != nil {
if outErr != nil && *outErr == nil {
*outErr = err
}
return
} else if !changed {
PanicOn(fmt.Sprintf("rbf.Tx.freePgno(): double free: %d", tx.pendingFreelistAdds))
}
}
}
}
// allocatePgno returns a page number for a new available page. This page may be
// pulled from the free list or, if no free pages are available, it will be
// created by extending the file size.
func (tx *Tx) allocatePgno() (uint32, error) {
func (tx *Tx) allocatePgno() (_ uint32, outErr error) {
if tx.modifyingFreelist {
return tx.allocateNewPgno(), nil
}
// Attempt to find page in freelist.
pgno, err := tx.nextFreelistPageNo()
if err != nil {
return 0, err
} else if pgno != 0 {
tx.modifyingFreelist = true
defer tx.freelistCleanup(&outErr)
c := Cursor{tx: tx}
c.stack.elems[0] = stackElem{pgno: readMetaFreelistPageNo(tx.meta[:])}
if changed, err := c.Remove(uint64(pgno)); err != nil {
@ -922,11 +994,16 @@ func (tx *Tx) allocatePgno() (uint32, error) {
}
return pgno, nil
}
// no freelist pages, fall back
return tx.allocateNewPgno(), nil
}
// allocateNewPgno requests a new page unconditionally, ignoring the free list.
func (tx *Tx) allocateNewPgno() uint32 {
// Increment the total page count by one and return the last page.
pgno = readMetaPageN(tx.meta[:])
pgno := readMetaPageN(tx.meta[:])
writeMetaPageN(tx.meta[:], pgno+1)
return pgno, nil
return pgno
}
func (tx *Tx) nextFreelistPageNo() (uint32, error) {
@ -952,10 +1029,16 @@ func (tx *Tx) nextFreelistPageNo() (uint32, error) {
}
// deallocate releases a page number to the freelist.
func (tx *Tx) freePgno(pgno uint32) error {
func (tx *Tx) freePgno(pgno uint32) (outErr error) {
if tx.modifyingFreelist {
tx.pendingFreelistAdds = append(tx.pendingFreelistAdds, pgno)
return nil
}
c := Cursor{tx: tx}
c.stack.elems[0] = stackElem{pgno: readMetaFreelistPageNo(tx.meta[:])}
tx.modifyingFreelist = true
defer tx.freelistCleanup(&outErr)
if changed, err := c.Add(uint64(pgno)); err != nil {
return err
} else if !changed {
@ -979,7 +1062,7 @@ func (tx *Tx) deallocateTree(pgno uint32) error {
return err
}
}
return nil
return tx.freePgno(pgno)
case PageTypeLeaf:
return tx.freePgno(pgno)

89
rbf/tx_test.go
View file

@ -22,6 +22,7 @@ import (
"time"
"github.com/molecula/featurebase/v2/rbf"
"github.com/molecula/featurebase/v2/roaring"
)
func TestTx_CommitRollback(t *testing.T) {
@ -245,12 +246,11 @@ func TestTx_DeallocateTree(t *testing.T) {
}
if err = tx.DeleteBitmap("x"); err != nil {
t.Fatal(err)
} else {
if ok, err := tx.Contains("x", 0); err != nil {
t.Fatal(err)
} else if ok {
t.Fatal("expected no value in recreated bitmap")
}
}
if ok, err := tx.Contains("x", 0); err != nil {
t.Fatal(err)
} else if ok {
t.Fatal("expected no value in recreated bitmap")
}
if err = tx.Check(); err != nil {
t.Fatalf("check: %v", err)
@ -369,6 +369,83 @@ func TestTx_Add_Quick(t *testing.T) {
})
}
func TestTx_DeallocateToFreeList(t *testing.T) {
db := MustOpenDB(t)
defer MustCloseDB(t, db)
tx := MustBegin(t, db, true)
defer tx.Rollback()
var err error
// Create bitmap & add value.
if err = tx.CreateBitmap("x"); err != nil {
t.Fatal(err)
}
if err = tx.CreateBitmap("y"); err != nil {
t.Fatal(err)
}
const N = 12274831
slots := make([]uint64, N)
for i := range slots {
slots[i] = uint64(i) << 10
}
bm := roaring.NewBitmap(slots...)
if _, err = tx.AddRoaring("x", bm); err != nil {
t.Fatal(err)
}
if err = tx.Check(); err != nil {
t.Fatal(err)
}
for i := 0; i < 500; i++ {
if _, err := tx.Add("y", uint64(i)<<16); err != nil {
t.Fatal(err)
}
}
if err = tx.Check(); err != nil {
t.Fatal(err)
}
if err := tx.DeleteBitmap("y"); err != nil {
t.Fatal(err)
}
if err = tx.Check(); err != nil {
t.Fatal(err)
}
if err = tx.Commit(); err != nil {
t.Fatal(err)
}
tx = MustBegin(t, db, true)
defer tx.Rollback()
// Delete bitmap, verifying that it's gone.
if err := tx.DeleteBitmap("x"); err != nil {
t.Fatal(err)
} else {
if ok, err := tx.Contains("x", 0); err != nil {
t.Fatal(err)
} else if ok {
t.Fatal("expected no value in recreated bitmap")
}
}
if err = tx.Check(); err != nil {
t.Fatal(err)
}
if err = tx.Commit(); err != nil {
t.Fatal(err)
}
tx = MustBegin(t, db, true)
defer tx.Rollback()
if err := tx.CreateBitmap("x"); err != nil {
t.Fatal(err)
}
if _, err := tx.AddRoaring("x", bm); err != nil {
t.Fatal(err)
}
if err = tx.Check(); err != nil {
t.Fatal(err)
}
if err = tx.Commit(); err != nil {
t.Fatal(err)
}
}
func TestTx_AddRemove_Quick(t *testing.T) {
if testing.Short() {
t.Skip("-short enabled, skipping")