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 setting _p_changed for small pure-Python BTrees. #11

Merged
merged 3 commits into from
Apr 7, 2015
Merged

Fix setting _p_changed for small pure-Python BTrees. #11

merged 3 commits into from
Apr 7, 2015

Conversation

jamadden
Copy link
Member

@jamadden jamadden commented Apr 7, 2015

When working on zopefoundation/zc.zodbdgc#1, we found a bug in the pure-Python BTree implementation. An elif that should have been its own if prevented the second and subsequent additions to a BTree from setting the _p_changed flag, right up until the first bucket needed to split.

This can cause data-loss for small BTrees.

This PR adds a test case and should fix the issue (the comment is copied from the C implementation).

@jamadden jamadden added the bug label Apr 7, 2015
@tseaver
Copy link
Member

tseaver commented Apr 7, 2015

This fix LGTM. I plan to experiment with the new test (verifying failure in py27-pure, success in py27) before merging. Too bad the darned test suite takes so long to run.

@jamadden
Copy link
Member Author

jamadden commented Apr 7, 2015

It definitely fails for me without the change and works with it, but of course feel free to confirm, extra eyeballs are always a good thing :)

[master]$ pypy `which nosetests` BTrees.tests.test_OLBTree:OLBTreeTest.testAddTwoSetsChanged
F
======================================================================
FAIL: testAddTwoSetsChanged (BTrees.tests.test_OLBTree.OLBTreeTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "BTrees/BTrees/tests/common.py", line 1121, in testAddTwoSetsChanged
    self.assertTrue(t._p_changed)
AssertionError: False is not true

----------------------------------------------------------------------
Ran 1 test in 0.001s

FAILED (failures=1)
[master]$ cd fixed
[fixed]$ pypy `which nosetests` BTrees.tests.test_OLBTree:OLBTreeTest.testAddTwoSetsChanged
.
----------------------------------------------------------------------
Ran 1 test in 0.001s

OK

On my machine at least, the Python 2.6 tests run almost twice as fast as many of the other versions (90s vs 193s for Py2.7). Travis runs them all about the same...I wonder why that is...

@tseaver
Copy link
Member

tseaver commented Apr 7, 2015

I forgot that the py27 tests would exercise the pure-Python stuff, too. I can definitely verify the fix.

tseaver added a commit that referenced this pull request Apr 7, 2015
Fix setting _p_changed for small pure-Python BTrees.
@tseaver tseaver merged commit 2835296 into zopefoundation:master Apr 7, 2015
@tseaver
Copy link
Member

tseaver commented Apr 7, 2015

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants