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

Wed Jul 25 00:04:10 UTC 2012

> On July 23, 2012, 10:30 a.m., Ralf Engels wrote:
> > Provided the auto test run through and if you are really feeling brave enought you can push it.

Okay, thanks. I've tested it again & again so let's hope it won't eat our users (it should not, worst case is that Amarok will abort on too eager assertion). I've put it to reviewboard mainly to get approval with the general direction.

- Matěj

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

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/0f468f2a/attachment.html>