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

Journal Checksums #2254

Merged
merged 4 commits into from Nov 2, 2018

Conversation

Projects
None yet
3 participants
@adamretter
Member

adamretter commented Nov 1, 2018

This PR adds a checksum to the Journal for each entry. The checksum of the entry is then compared when reading the Journal back for recovery. This enables us to ensure that the Journal file is not corrupt.

I have used the xxhash-64 algorithm from the lz4 project, which in my testing for the typical size of journal entries was faster than lz4's xxhash-32, Java's Adler32, and OpenHFT's no-allocation xxhash.

fixed-hash-benchmarks

NOTE This PR also increments the Journal file format version to version 3.

@adamretter adamretter added this to the eXist-4.5.0 milestone Nov 1, 2018

@adamretter adamretter requested review from wolfgangmm and dizzzz Nov 1, 2018

@adamretter adamretter modified the milestones: eXist-4.5.0, eXist-5.0.0 Nov 1, 2018

This was referenced Nov 1, 2018

"; transactId = " + transactId);
throw new LogException("Bad pointer to previous in entry: " + loggable.dump());
}

// update the checksum for the entry data and backLink
if (payload.hasArray()) {

This comment has been minimized.

@dizzzz

dizzzz Nov 1, 2018

Member

looks like almost identical code of line 211 ? I can imagine we want to avoid the cost of making a call to a new method?

This comment has been minimized.

@adamretter

adamretter Nov 2, 2018

Member

I could abstract it, but I would need access to several local vars, as it was small I just reproduced it.

@dizzzz

wow, serious stuff. I get the big picture, LGTM. Wondering if a journal is not completely written due e.g. a JVM crash, will the loader robust for this? wouldn't it be safer to write the checksum before the larger payload data in writeToLog() ? how large can the payload become?

@dizzzz

This comment has been minimized.

Member

dizzzz commented Nov 1, 2018

see my remark; I'd guess some longer stress testing might needed here?

@wolfgangmm

Looks good to me. I cannot remember having seen a corrupted journal yet, but it's hard to know, so having a check is better than not having one ;-)

@adamretter

This comment has been minimized.

Member

adamretter commented Nov 2, 2018

@dizzzz some replies to your comments:

Wondering if a journal is not completely written due e.g. a JVM crash, will the loader robust for this?

No. At the moment, we just detect the failure during recovery and abort. However, recovery will then only have been done as far as the checksum failure. This could lead to inconsistencies in the recovered data. Consider this an improvement on what we had, whereby previously we might ignore journal corruptions and report that recovery completed.

For future improvements, we have two other options which would be more robust:

  1. Do a pre-scan of each journal file before recovery, checking that the checksums are valid.
  2. and/or switch to a fixed block size based journal format such as that used in LevelDB, this enables us to more easily detect corruptions due to the framing.

wouldn't it be safer to write the checksum before the larger payload data in writeToLog() ?

I am not sure it makes much difference. If there is a corruption, regardless of whether the checksum is before or after the payload, it will not match.

how large can the payload become?

The older journal code is a bit yucky and won't cope with payloads greater than 32 KB. In reality the payloads written by eXist-db are always under 4 KB which is the B-Tree page size.

@dizzzz dizzzz merged commit ac5ebfa into eXist-db:develop Nov 2, 2018

1 check failed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details

@adamretter adamretter deleted the adamretter:feature/journal-checksums branch Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment