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

Provide a way to truncate a corrupted Badger DB #5275

Closed
schomatis opened this issue Jul 22, 2018 · 11 comments
Closed

Provide a way to truncate a corrupted Badger DB #5275

schomatis opened this issue Jul 22, 2018 · 11 comments
Assignees
Labels
kind/bug A bug in existing code (including security flaws) topic/badger Topic badger topic/docs-ipfs Topic docs-ipfs

Comments

@schomatis
Copy link
Contributor

Badger needs explicit consent from the user to truncate a corrupted DB when starting: passing a --truncate flag. That flag is not being provided as an option right now and the user gets errors like:

# IPFS_PATH=/ipfs/ipfs_master/repo ./ipfs daemon
Initializing daemon...
Error: Unable to replay value log: "/ipfs/ipfs_master/repo/badgerds/000017.vlog": Data corruption detected. Value log truncate required to run DB. This would result in data loss.
Received interrupt signal, shutting down...
(Hit ctrl-c again to force-shutdown the daemon.)

(and #5213 (comment)), which are not really useful.

Documentation should also be added as it's a normal scenario to have a (relatively small) portion of the DB corrupted after a crash.

@schomatis schomatis added kind/bug A bug in existing code (including security flaws) topic/docs-ipfs Topic docs-ipfs topic/badger Topic badger labels Jul 22, 2018
@schomatis schomatis self-assigned this Jul 22, 2018
@Stebalien
Copy link
Member

Will this small amount include "synced" data? If so, and if there's no way for us to actually sync the data, we should move to a different datastore. If not, we should just truncate by default.

Explicitly, I'd expect the following semantics:

  1. If Datastore.Put has returned, the data must be persisted. Passing the truncate option shouldn't remove this data.
  2. If Datastore.Put has not returned, we don't care. Passing the truncate option may remove this data.

@schomatis
Copy link
Contributor Author

Will this small amount include "synced" data?

It shouldn't if syncWrites is set (which by default it is), as everything will be directly persisted to the value log on disk, and from that point it can be recovered after a crash. However, if that is a deal breaker, let me double check that and get back to you.

@Stebalien
Copy link
Member

We need some commit point. Some point where we can know that the data has been persisted.

@schomatis
Copy link
Contributor Author

Sorry for the delay here, completing @bigs notes from the Dev meeting, if syncWrites is set and I commit the transaction the following should happen:

The Commit will guarantee that all the entries will passed to the valueLog structure (flushing pendingWrites). The valueLog will write a buffer filled with those entries in a Go file opened with the O_DSYNC flag. In case a system error occurs after committing the transaction, when opening the DB the system will be able to replay at least up to the last entry of that transaction in the SSTables while iterating the value log.

(@manishrjain Feel free to ignore this but if you can would you mind validating that what I'm saying in this last paragraph is correct?)

@manishrjain
Copy link

Yes, that understanding is correct. If it was applied successfully to the value log, it would be replayed back after crash.

@Stebalien
Copy link
Member

So, it looks like, given the semantics we expect, we should always enable truncation if SyncWrites is enabled.

PR: ipfs/go-ds-badger#30

@Stebalien
Copy link
Member

@schomatis do you have time to fix this as we discussed? That is:

  1. Add a truncate config option.
  2. Turn it on by default.

@schomatis
Copy link
Contributor Author

Yes, I'll try to get to it next week.

Turn it on by default.

Could you point me to the part of the code that generates the config file please?

@Stebalien
Copy link
Member

@Stebalien
Copy link
Member

@djdv or @schomatis can we try to move forward on this?

@schomatis
Copy link
Contributor Author

Yes, sorry, totally forgot about this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) topic/badger Topic badger topic/docs-ipfs Topic docs-ipfs
Projects
None yet
Development

No branches or pull requests

3 participants