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

fix rollback panic bug #153

Merged
merged 1 commit into from
Jun 8, 2019
Merged

fix rollback panic bug #153

merged 1 commit into from
Jun 8, 2019

Conversation

WIZARD-CXY
Copy link
Contributor

fix the panic bug in freelist rollback function and add test.

@WIZARD-CXY
Copy link
Contributor Author

ref #152

@WIZARD-CXY
Copy link
Contributor Author

Since I don't see the code testing the Rollback function when syncFreelist is true or false. I add both.

@WIZARD-CXY
Copy link
Contributor Author

@kozlovic @xiang90

@xiang90
Copy link
Contributor

xiang90 commented Mar 13, 2019

@WIZARD-CXY Is there a less expensive way to rollback the tx? Can we simply put back the allocated pages for the write tx when rolling back?

@WIZARD-CXY
Copy link
Contributor Author

WIZARD-CXY commented Mar 13, 2019

@xiang90 yeah, I have the same concern. Now freelist rollback kinda what u want. But Tx rollback now call freelist rollback and reload two functions. I think reload which is heavy maybe unnecessary.. but not 100% sure. So I added it for now.

@WIZARD-CXY
Copy link
Contributor Author

WIZARD-CXY commented Mar 13, 2019

If we want to keep the exact same behavior as the sync freelist version for the nosyncfreelist, we must fully load the whole freelist.

@WIZARD-CXY
Copy link
Contributor Author

Actually, I tried to remove the reload function in tx.Rollback the boltdb test passed

@WIZARD-CXY
Copy link
Contributor Author

WIZARD-CXY commented Mar 14, 2019

@benbjohnson why we call freelist's reload in tx Rollback function, can we remove it?

@WIZARD-CXY
Copy link
Contributor Author

@xiang90 @yudai @gyuho @hexfusion ptal

@WIZARD-CXY
Copy link
Contributor Author

kindly ping @xiang90 @yudai @gyuho @hexfusion

tx.go Outdated Show resolved Hide resolved
freelist.go Outdated Show resolved Hide resolved
@WIZARD-CXY
Copy link
Contributor Author

@hormes changed ptal @xiang90

freelist.go Outdated Show resolved Hide resolved
@WIZARD-CXY
Copy link
Contributor Author

@xiang90 @hormes ptal

tx.go Outdated Show resolved Hide resolved
@@ -349,6 +349,28 @@ func (f *freelist) reload(p *page) {
f.readIDs(a)
}

// noSyncReload reads the freelist from pgids and filters out pending items.
func (f *freelist) noSyncReload(pgids []pgid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

freelist knows its own type. we can do the branch inside the reload func.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type of freelist is hashmap or array. It is irrelevant with sync or not-sync.

@xiang90
Copy link
Contributor

xiang90 commented Jun 6, 2019

just two styling issue.

@xiang90
Copy link
Contributor

xiang90 commented Jun 8, 2019

@WIZARD-CXY

I did a release here https://github.com/etcd-io/bbolt/releases/tag/v1.3.3. can you update the dependency of bbolt in etcd to include the fix?

@WIZARD-CXY
Copy link
Contributor Author

@xiang90 consider it done

thaJeztah added a commit to thaJeztah/containerd that referenced this pull request Aug 9, 2019
this brings the dependency back to a released version:

- go.etcd.io/bbolt etcd-io/bbolt@2eb7227...v1.3.3
  - etcd-io/bbolt#153 fix rollback panic bug
    - fixes etcd-io/bbolt#152 Panic (index out of range) on writeable tx rollback with db.NoFreelistSync

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
tussennet pushed a commit to tussennet/containerd that referenced this pull request Sep 11, 2020
this brings the dependency back to a released version:

- go.etcd.io/bbolt etcd-io/bbolt@2eb7227...v1.3.3
  - etcd-io/bbolt#153 fix rollback panic bug
    - fixes etcd-io/bbolt#152 Panic (index out of range) on writeable tx rollback with db.NoFreelistSync

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants