diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index 82710bb4d..f94cbcc0e 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -221,16 +221,17 @@ deploy node for linux amd64: - echo "$AWS_SSH_PRIVATE_KEY" | ssh-add - - chmod 700 /root/.ssh - '[[ -f /.dockerenv ]] && echo -e "Host *\n\tStrictHostKeyChecking no\n\n" > ~/.ssh/config' + - apt update && apt -y install jq script: - AMI=$(aws ssm get-parameters --names /aws/service/ami-amazon-linux-latest/amzn2-ami-hvm-x86_64-ebs --query 'Parameters[0].[Value]' --output text --profile $PROFILE) - SECURITY_GROUP=$(aws ec2 describe-security-groups --filters Name=vpc-id,Values=vpc-03a4ba3d5b7c8f978 Name=group-name,Values=default --query 'SecurityGroups[*].[GroupId]' --output text --profile $PROFILE) - SUBNET_ID=$(aws ec2 describe-subnets --filters 'Name=vpc-id,Values=vpc-03a4ba3d5b7c8f978' 'Name=availability-zone,Values=us-east-2a' --query 'Subnets[0].SubnetId' --output text --profile $PROFILE) - - aws ec2 run-instances --image-id $AMI --instance-type $INSTANCE --security-group-ids $SECURITY_GROUP --subnet-id $SUBNET_ID --key-name gitlab-featurebase-dev --tag-specifications 'ResourceType=instance,Tags=[{Key=Name,Value=linux-amd64-node}]' --profile $PROFILE --user-data file://./qa/scripts/cloud-init.sh --iam-instance-profile Name=featurebase-dev-ssm - - sleep 120 # Need to wait for the EC2 instance to launch and run the initialization commands passed through user-data - - PUBLIC_IP=$(aws ec2 describe-instances --filters 'Name=tag:Name, Values=linux-amd64-node' 'Name=instance-state-name, Values=running' --query 'Reservations[*].Instances[*].PublicIpAddress' --output text --profile $PROFILE) - - echo $PUBLIC_IP - - INSTANCE_ID=$(aws ec2 describe-instances --filters 'Name=tag:Name, Values=linux-amd64-node' 'Name=instance-state-name, Values=running' --query 'Reservations[*].Instances[*].InstanceId' --output text --profile $PROFILE) + - aws ec2 run-instances --image-id $AMI --instance-type $INSTANCE --security-group-ids $SECURITY_GROUP --subnet-id $SUBNET_ID --key-name gitlab-featurebase-dev --tag-specifications 'ResourceType=instance,Tags=[{Key=Name,Value=linux-amd64-node}]' --profile $PROFILE --user-data file://./qa/scripts/cloud-init.sh --iam-instance-profile Name=featurebase-dev-ssm > config.json + - INSTANCE_ID=$(jq '.Instances | .[] |.InstanceId' config.json | tr -d '"') - echo $INSTANCE_ID > linux-amd64-instance.txt + - sleep 120 # Need to wait for the EC2 instance to launch and run the initialization commands passed through user-data + - PUBLIC_IP=$(aws ec2 describe-instances --instance-ids $INSTANCE_ID --filters 'Name=instance-state-name, Values=running' --query 'Reservations[*].Instances[*].PublicIpAddress' --output text --profile $PROFILE) + - echo $PUBLIC_IP - scp -o StrictHostKeyChecking=no -i gitlab-featurebase-dev.pem featurebase_linux_amd64 ec2-user@$PUBLIC_IP:. - scp -o StrictHostKeyChecking=no -i gitlab-featurebase-dev.pem ./qa/scripts/featurebase.conf ./qa/scripts/featurebase.service ec3-user@$PUBLIC_IP:. - aws ssm send-command --document-name "AWS-RunShellScript" --instance-ids $INSTANCE_ID --cli-input-json file://./qa/scripts/configureFeatureBase.json --profile $PROFILE --region us-east-2 diff --git a/rbf/tx.go b/rbf/tx.go index 9ecbe9a02..812d35738 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -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) diff --git a/rbf/tx_test.go b/rbf/tx_test.go index f7cd78d6a..e4b68f4b6 100644 --- a/rbf/tx_test.go +++ b/rbf/tx_test.go @@ -22,6 +22,7 @@ import ( "time" "github.com/molecula/featurebase/v2/rbf" + "github.com/molecula/featurebase/v2/roaring" ) func TestTx_CommitRollback(t *testing.T) { @@ -219,6 +220,95 @@ func TestTx_DeleteBitmap(t *testing.T) { } } +// deallocateTree had a bug which caused a page to be marked neither free +// nor in-use. +func TestTx_DeallocateTree(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) + } + const N = 315 + slots := make([]uint64, N) + for i := range slots { + slots[i] = uint64(i) << 20 + } + if _, err = tx.Add("x", slots...); err != nil { + t.Fatal(err) + } + if err = tx.Check(); err != nil { + t.Fatalf("check: %v", err) + } + if err = tx.DeleteBitmap("x"); err != nil { + t.Fatal(err) + } + 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) + } +} + +func TestTx_RecreateBitmap(t *testing.T) { + db := MustOpenDB(t) + defer MustCloseDB(t, db) + tx := MustBegin(t, db, true) + defer tx.Rollback() + + // Create bitmap & add value. + if err := tx.CreateBitmap("x"); err != nil { + t.Fatal(err) + } + const N = 825000 + slots := make([]uint64, N) + for i := range slots { + slots[i] = uint64(i) << 20 + } + if _, err := tx.Add("x", slots...); err != nil { + t.Fatal(err) + } + err := tx.Commit() + if 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") + } + } + err = tx.Commit() + if 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.Add("x", slots...); err != nil { + t.Fatal(err) + } + err = tx.Commit() + if err != nil { + t.Fatal(err) + } +} + func TestTx_RenameBitmap(t *testing.T) { db := MustOpenDB(t) defer MustCloseDB(t, db) @@ -279,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")