Clementine should detect a corrupt sqlite3 database and offer to fix or delete it

[Clementine-Issue-Importer]

From ma...@juffo.org on February 23, 2012 21:28:02

Whenever I type something to the music search bar, I get a window pop up with the error: LibraryBackend: database disk image is malformed Unable to fetch row

This started occuring after my computer lost power unexpectedly. As far as I know, Clementine shouldn't have been writing to the database around this moment. It was in the middle of a long (2-hour) track; I hadn't touched it for a while and didn't change the music files/directories on disk either.

Everything else, besides the search, seems to be working fine.

I read somewhere that Clementine uses some external SQLite module for text searches. Maybe this search index isn't crash-safe? If so, Clementine should be able to automatically recover and reindex when detecting such corruption.

Or at the very least, Clementine should make automated backups of the metadata database files. What version of the product are you using? On what operating system? Clementine 1.0.1, on Arch Linux testing, x86_64.

Original issue: http://code.google.com/p/clementine-player/issues/detail?id=2743

[Clementine-Issue-Importer]

From john.maguire on March 12, 2012 09:37:46

The effort involved here far outweighs the benefit.

Status: WontFix

[Clementine-Issue-Importer]

From arnaud.bienner on March 12, 2012 09:57:09

hmmm... Are your sure about the "WontFix"? Problems with database corruptions are recurrent and, above all, very hard to fix for non advanced users, as this requires to understand the error, find the database, and remove it: basically, users will jut stop using Clementine. Having a mechanism that would ignore errors and correct them (in the worst way, by automatically removing the old database, and, in a better way, trying to correct only bad entries) would be nice. Another way to solve the problem would be to remove the database when uninstalling Clementine (as I thing people having problems will be likely to try to uninstall then reinstall the software to try to fix the issue).

IMHO, I think we can reopen this issue, as an enhancement request with low-priority: what do you think about this?

@marti@juffo.org: could you please attach to this issue the corrupted database, to help us for investigation?

[Clementine-Issue-Importer]

From ma...@juffo.org on March 12, 2012 10:12:54

There have been 3 reports of this issue (#2421, #2625 and #2743), a forum thread ( http://www.pclinuxos.com/forum/index.php?topic=101337.0 ) and the only proposed solution is to "delete all your metadata" and you just dismiss the issue as "not worth the benefit" without even any dialogue about what the options are?

Computer crashes/power losses are a fact of life and there's no way around it.

Let me propose 3 solutions, none of which seem like much coding effort:

1. SQLite claims that it's crash-safe. We could collect some samples of corrupted databases and send them to SQLite developers so they could investigate and hopefully fix the issue.
2. Create a backup (file copy) of the database on every startup and keep around 5 or so last backups. Manual work would still be necessary to restore it, but at least it's possible.
3. Add an option to simply rebuild the search index.

In any case, closing reports that are clearly bugs is a horrble way to communicate with your users. At the very least keep this bug open as an acknowledgement and a way for users to exchange workarounds for the issue.

[Clementine-Issue-Importer]

From ma...@juffo.org on March 12, 2012 11:15:17

The crashed database is attached (gzipped). Apparently the songs_fts_segdir table became corrupted; other tables seem to be intact.

sqlite> select * from songs_fts_segdir; Error: database disk image is malformed sqlite> select count(*) from songs_fts_segdir; 4 sqlite> drop table songs_fts_segdir; Error: database disk image is malformed

But I managed to recover most of it with a full dump & restore. Here's what I did: cd ~/.config/Clementine mv clementine.db clementine.db.crashed sqlite3 clementine.db.crashed .dump > dump.sql

edit the dump.sql file, change last line from ROLLBACK; to COMMIT;

sqlite3 clementine.db < dump.sql

After restoring, the corrupted table is now empty. I can't search for songs that were in the database before (even a full rescan doesn't help), but newly added songs are searchable.

Attachment: clementine.db.crashed.gz

[Clementine-Issue-Importer]

From davidsansome on March 12, 2012 11:24:39

Ok I agree that Clementine should try to do something better with corrupted databases - even if it is to pop up a dialog asking the user whether they want to delete the whole thing and start over.

Summary: Clementine should detect a corrupt sqlite3 database and offer to fix or delete it
Status: Accepted
Labels: -Type-Defect Type-Enhancement

[Clementine-Issue-Importer]

From ma...@juffo.org on March 13, 2012 09:32:37

I wrote an initial attempt at making an integrity check and database backups at startup. It's available in my GitHub repository: https://github.com/intgr/clementine/commits/ The code isn't exactly pretty, since I need to use SQLite's internal C API to make the backup -- a plain file copy backup could be inconsistent (i.e. corrupt). Qt's SQLite wrappers don't implement the backup API.

Because the Database object already uses SQLite C internal functions, I decided to add mine there too for consistency's sake. The backup and integrity check is done in the database thread, so it doesn't delay Clementine's startup too much.

Since this is my first attempt at hacking on Clementine and Qt code, there are probably a million things wrong with my code. Any feedback would be appreciated. :)

[Clementine-Issue-Importer]

From arnaud.bienner on March 13, 2012 17:30:13

I just has a quick look to this, but it looks very promising :) Just one thing that shocked me: the use of "goto" instruction: I thought almost nobody use it today. http://en.wikipedia.org/wiki/Goto#Criticism_and_decline Just out of curiosity, why just copying the database file isn't enough? What sqlite3's backup function does different?

Would be nice also to have the ability to restore an old database: in case of error, just need to ask the user if he wants to (and if he wants to use a brand new database). Maybe it would be nice also to have several backups files and, in that case, give the user the ability to choose the one he would prefer to restore. But these ideas can (should?) be implemented in a second step I think.

Labels: PatchAttached

[Clementine-Issue-Importer]

From ma...@juffo.org on March 13, 2012 18:44:15

Sorry, this comment is long-winded.

Would be nice also to have the ability to restore an old database Agreed, I will look into it if I find the time.

it would be nice also to have several backups files Yeah, I was planning to add that.

why just copying the database file isn't enough? Since the copy isn't atomic -- it would cause inconsistencies when there are concurrent modifications of the database (which is likely during the startup, since that's when the library updating happens)

Consider:

1. Backup copies block A
2. Thread updates block A to A'
3. Thread updates block B to B'
4. Backup copies block B'

So from the writer's perspective, consistent states are (A B), (A' B) or (A' B') -- since that's the order they were written. But the backup contains (A B') -- as if the latter write occured, but the earlier didn't.

Just one thing that shocked me: the use of "goto" instruction C coders do use it for cleanup blocks, it's a poor man's version of try{}finally{}. If I didn't use goto, I'd have to either nest the success cases in mutiple levels of if()s, or duplicate the cleanup logic in every error condition. Both would be a loss in readability IMO.

Some resources advocating gotos for this purpose:

• http://kerneltrap.org/node/553/2131 * https://www.securecoding.cert.org/confluence/display/seccode/MEM12-C.+Consider+using+a+Goto-Chain+when+leaving+a+function+on+error+when+using+and+releasing+resources That said, if it doesn't comply with Clementine's coding style, I'm open to changing it.

[Clementine-Issue-Importer]

From arnaud.bienner on March 14, 2012 05:30:28

Issue 2421 has been merged into this issue.

[Clementine-Issue-Importer]

From john.maguire on March 15, 2012 08:02:38

This issue was updated by revision 9cf279f5a3e6 .

Integrity check now run on startup

[Clementine-Issue-Importer]

From john.maguire on March 15, 2012 08:05:25

Marti, I committed your patch for running the integrity check (with some style changes). I'm less sure about the backup patch, could you at least update it to follow our style, ie. declarations next to usages, RAII (Boost's scope exit might be useful here) and then I'll take another look.

[Clementine-Issue-Importer]

From john.maguire on March 16, 2012 06:53:01

This issue was updated by revision 9ed8ab63aa2a .

Database backup now run on startup, still needs a way to restore.

[Clementine-Issue-Importer]

From ma...@juffo.org on March 16, 2012 07:17:52

Thanks! Sorry, I was planning to rewrite it, but didn't find the time yet.

PS: The error message is misleading. When opening the destination database fails, you will report "Failed to open source database".

[thearthur]

The backup just saved me, after i added a song to the playlist that causes Clementine to segfault on startup. because I had the backup, I was able to get it unstuck without loosing my setup. The presence of a db.bak file was enough of a clue to figure this out even without any restore feature.

[Zulgrib]

At this day, the backup can get erased by corrupted database too easily and the backup is useless in most cases because erased before you understand what is happening. If Clementine was not correctly closed last time, it should restore from backup by default.