Review Request: SqlScanResultProcessor: fix data-loss bug; squashed commits, recent on top

Wed Jul 25 00:21:45 UTC 2012

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105488/#review16328
-----------------------------------------------------------

This review has been submitted with commit 514d40d2b2fccf3951541be67d7be9534ec6d9a9 by Matěj Laitl to branch master.

- Commit Hook

On July 25, 2012, 12:01 a.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105488/
> -----------------------------------------------------------
> 
> (Updated July 25, 2012, 12:01 a.m.)
> 
> 
> Review request for Amarok and Ralf Engels.
> 
> 
> Description
> -------
> 
> SqlScanResultProcessor: cope with non-unique uniqueid in the database
> 
> Unfortunately, the uniqueid column (or rather its index) of our urls
> table is not defined as unique and unfortunately at least some code in
> SqlCollection doesn't check for duplicates before inserting to the
> table. This can be provoked for example by using the "Organize
> Collection" functionality.
> 
> While fixing SqlCollectionLocation in short-term and making the
> uniqueid index unique in long-term is probably needed, we need to cope
> with existing user databases. This change is needed because
> SqlScanResultProcessor identified tracks fully by their uniqueid which
> resulted in unpredictable and incorrect behaviour - it for example
> never removed the "old" duplicate entry in deleteDeletedTracks( int )
> and sometimes found incorrect entry when importing a track in
> commitTrack().
> 
> This does not solve bug 289338, but it dramatically reduces its
> consequences, the (correct) duplicates are removed as soon as
> collection scanner fires.
> 
> v2: the test failure spotted by Sentynel discovered a bug in patch v1,
> there was an assertion that sometimes failed even for normal operation
> because database updates were temporarily blocked. Fix by moving the
> assertion to a place where it is valid in all cases.
> 
> CCBUG: 289338
> 
> SqlScanResultProcessor: skip removeTrack() if there were errors
> 
> Another in a series that try to minimize chance of users losing their
> tracks, statistics etc.
> 
> SqlScanResultProcessor: don't accidentally delete tracks, defensive rewrite
> 
> This fixes data-loss that I can trigger every time by toggling "Local
> Files & USB Mass Storage Backend" in Config -> Plugins, restarting
> Amarok and triggering collection update / rescan.
> 
> In theory, more things such as cloning changing disk could trigger this
> problem, from SqlScanResultProcessor::deleteDeletedDirectories() comment:
> 
> We need to match directories by their (absolute) path, otherwise following
> scenario triggers statistics loss (bug 298275):
> 
> 1. user relocates collection to different filesystem, but clones path structure
>    or toggles MassStorageDeviceHandler enabled in Config -> plugins.
> 2. collectionscanner knows nothings about directory ids, so it doesn't detect
>    any track changes and emits a bunch of skipped (unchanged) dirs with no
>    tracks.
> 3. SqlRegistry::getDirectory() called there from returns different directory id
>    then in past.
> 4. deleteDeletedDirectories() is called, and if it operates on directory ids,
>    it happily removes _all_ directories, taking tracks with it.
> 5. Tracks disappear from the UI until full rescan, stats, lyrics, labels are
>    lost forever.
> 
> Also add a handful of asserts, ScanResultProcessor is very complicated
> and small error or corner-case in logic may result in horrible data
> losses.
> 
> Reporters of linked bugs, please try to reproduce your data-loss with
> this patch applied and report back in both cases. In negative case,
> please reopen and attach full updated amarok --debug log.
> 
> After this patch, only (statistics, lyrics and labels)-loss operations
> should be:
>  a) moving track out of mounted collection folders [by design]
>  b) changing both metadata and url from outside of a track not tagged
>     by amarok_afttagger [we can do nothing about this]
> 
> BUG: 298275
> FIXED-IN: 2.6
> 
> SqlMeta::Track: remove unused methods deviceId() and rpath()
> 
> There are unused and rather internal, to remove them.
> id() and urlId() are unused, too, but these are at least
> in theory useful.
> 
> SqlScanResultProcessor: remove dead code, sanitize includes
> 
> 
> SqlRegistry, DatabaseUpdater: delete stats and everything in removeTrack()
> 
> SqlRegistry::removeTrack() had code to remove the entry from the tracks
> table and to preserve the entry in the statistics table. This doesn't
> work, because SqlRegistry construction calls
> DatabaseUpdater::deleteAllRedundant( url ) on next start-up that
> removes the url entry, plus we don't know how long we should keep the
> entry, so we just delete everything. ScanResultProcessor should be
> witty enough not to delete tracks that have been moved to another
> directory and/or device, even if it is currently unavailable.
> 
> Additionally, we clean up the statistics, urls_labels, lyrics tables
> on start-up to avoid stale entries (pointing to deleted url) in a way
> similar to what it is done with other tables.
> 
> TestSqlScanManager is updated so that it doesn't test removed
> behaviour.
> 
> SqlScanResultProcessor: rename attributes in preparation for a fix
> 
> ...because I was really confused by the old names.
> 
> 
> This addresses bug 298275.
>     https://bugs.kde.org/show_bug.cgi?id=298275
> 
> 
> Diffs
> -----
> 
>   ChangeLog 657219807f7bc3ca360d786e46503549cb1a5663 
>   src/core-impl/collections/db/sql/DatabaseUpdater.h 57af65de0e3cbea5d7b7693f2d586eb1fe823870 
>   src/core-impl/collections/db/sql/DatabaseUpdater.cpp 59e071d2f4fc69d79196e22e9119ff02805f28ca 
>   src/core-impl/collections/db/sql/SqlMeta.h c4242a403c11fd37ea15d8c0d8c0e781d4ee1541 
>   src/core-impl/collections/db/sql/SqlMeta.cpp cb95a138da4cbfa97e9777bb085000db31024123 
>   src/core-impl/collections/db/sql/SqlRegistry.h 4bbe0de31ac742ce3f8bcc41f338f5e9bceb3daa 
>   src/core-impl/collections/db/sql/SqlRegistry.cpp 1b9efe5490bebc849b6c52c0b0b3fd51a5565418 
>   src/core-impl/collections/db/sql/SqlScanResultProcessor.h 1bd96db03363cf8f760811fb231fd9facf521e8e 
>   src/core-impl/collections/db/sql/SqlScanResultProcessor.cpp 689b9006280dba7061a9bd1bf4545fd44ef34106 
>   tests/core-impl/collections/db/sql/TestSqlScanManager.cpp 3974b1d234302110d0e00121be3601a06471163f 
> 
> Diff: http://git.reviewboard.kde.org/r/105488/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120725/356d16c8/attachment-0001.html>