FileStorage: fix rare data corruption when using restore after multiple undos

[perrinjerome]

The case of a history like this:

• T0 initialize an object in state 0
• T1 modifies object to state 1
• T2 modifies object to state 2
• T3 undo T2 and T1, bringing back to state 0
• T4 modified object to state 3
• T5 undo T4, bringing back object to state 0

was not correct after restore: the state is 1 instead of the expected 0.

This is because T3 contains two data records:

• an undo of T2, with a backpointer to the data of state 1
• an undo of T1, with a backpointer to the data of state 0

When restoring T5 (the undo of T4), the transaction is made of one data record, with a backpointer that is copied from the backpointer from T3, but this uses backpointer from the first record for this object, which is incorrect, in such a case where there is more than one backpointer for the same oid, we need to iterate in all data record to find the oldest version.

[perrinjerome]

For reference, the file format is described in https://github.com/zopefoundation/ZODB/blob/6f8878124fa8bf1cc1932a8f67da8d15b653897c/src/ZODB/FileStorage/format.py#L15-L84

[d-maurer]

I very much prefer PRs from the main repository (rather than from forks): for difficult code, it is important to see the modification in a larger context and not only the diff. To support this, I have cloned the main repository and do not like to add additional clones (or "remote"s) for reviews.

[navytux]

Hello Dieter. I understand your rationale, and on my side try to follow your advice after you expressed the same reasoning to me. But just for the reference, if you add the following into ZODB/.git/config

[remote "origin"]
        url = https://github.com/zopefoundation/ZODB.git
        fetch = +refs/heads/*:refs/remotes/origin/*
        fetch = +refs/pull/*/head:refs/remotes/origin/pr/*          <-- this line

you will be able to check out code of all pull requests without adding any remote or something. For example with

hereby PR it would be git fetch origin; git checkout origin/pr/395.

[d-maurer]

Jérome Perrin wrote at 2024-2-1 05:40 -0800:

... I can make another pull request to change all these to use skip, unless someone here thinks it's bad ?

unittest used to have problems when dubug was called for a skipped method. With zope.testrunner debug is called with the "-D" option.

When zope.testrunner's -D option is used, it is expected that it runs until the first failure and there enters the debugger (allowing the tester to analyse the failure context). Due to the above problem, the first encountered skip was considered a failure and entered the debugger.

I filed a corresponding Python bug report but I am not sure whether a fix is in all supported Python versions. Should the problem still exist, I would not massively use skip.

[perrinjerome]

Ah yes, nowadays zope.testrunner's -D no longer enter debugger on skipped tests ( was fixed in https://github.com/zopefoundation/zope.testrunner/issues/141 ) , but there is still something that teardown is not always executed ( https://github.com/zopefoundation/zope.testrunner/pull/142#issuecomment-1350430617 ), so it seems still safer to just keep using the "pass" pattern.

[d-maurer]

Jérome Perrin wrote at 2024-2-14 07:23 -0800:

... @perrinjerome commented on this pull request.

@@ -672,28 +672,33 @@ def _data_find(self, tpos, oid, data): tid, tl, status, ul, dl, el = unpack(TRANS_HDR, h) status = as_text(status) self._file.read(ul + dl + el)

• tend = tpos + tl + 8
• tend = tpos + tl

I tried to write a test calling restore with a txn that did not contain the oid, it triggers the problem, but it does not really represent a normal scenario so I'm not 100% sure what to do.

I think, we do not need a test to fix the + 8 bug. I know that I asked about the "why remove + 8" but our discussion convinced me that it is the right thing: According to "format.py", tl contains "transaction record length - 8" and a transaction record ends with a sequence of "data record"s followed by "8-byte redundant transaction length -8". This means that the last data record ends at tpos + tl, not tpos + tl + 8.

[navytux]

@perrinjerome, thanks for adding the test and robustifying handling of records reader.

@d-maurer, thanks also for confirming you are ok with the fix and we all now share common understanding.

I think that tests are added not because the code is wrong, but to preserve it to stay working correctly. So in general whenever we fix something, we try to add a test for the fixed problem so that we know for sure it won't return after follow-up changes.

Sometimes it might be hard to do a test and the difficulties outweight usefulness of having tests. But in this case we already have tests done as part of https://github.com/zopefoundation/ZODB/pull/397 so unless there is a strong reason not to take them, let's just add both the fix and the test to make sure that quality is improved and stays ok.

Anyway, besides the tests, am I right that we all three agree that this patch is ok now? Or did I missed something and there is still something else to be improved?

Kirill

[d-maurer]

Kirill Smelkov wrote at 2024-2-16 08:25 -0800:

... @d-maurer, thanks also for confirming you are ok with the fix and we all now share common understanding.

I think that tests are added not because the code is wrong, but to preserve it to stay working correctly. So in general whenever we fix something, we try to add a test for the fixed problem so that we know for sure it won't return after follow-up changes.

In this particular case, it is very unlikely that someone will readd the + 8 again and change the loop logic otherwise. But, I am not against a test if someone has already spent time on it.

[navytux]

Dieter, thanks for feedback. What you say is reasonable.

[perrinjerome]

I feel that we agreed on merging this after merging https://github.com/zopefoundation/ZODB/pull/397 , I "requested review" from github interface, if you agree please approve the review and I'll apply the changes.