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

Wed Jul 11 15:46:43 UTC 2012

> On July 11, 2012, 9:20 a.m., Ralf Engels wrote:
> > Looks good for now.
> > What I would really like is some more test cases to check for the current problems.
> > 
> > Also I am working on a solution for the database schema as you can see from the database table .svg file in the docs directory.

> Looks good for now.
> What I would really like is some more test cases to check for the current problems.

Agreed, but due to their complexity, I won't be able to write them in Amarok 2.6 timeframe. Do you think we can merge this for 2.6 even without tests?

> Also I am working on a solution for the database schema as you can see from the database table .svg file in the docs directory.

Yeah, I've seen it and generally it's a right step forward IMO. But please let me look at it in more detail after 2.6 is released. Quick question: don't you think that multiple artists (etc) for a track will make code much more complicated?

- Matěj

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

On July 10, 2012, 2:54 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105488/
> -----------------------------------------------------------
> 
> (Updated July 10, 2012, 2:54 p.m.)
> 
> 
> Review request for Amarok and Ralf Engels.
> 
> 
> Description
> -------
> 
> SqlScanResultProcessor: debugging; not intended to be merged to master
> 
> 
> 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().
> 
> 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
> REVIEW: 105488
> 
> 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.
> 
> 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
> -----
> 
>   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 8ee1014eb160547c4cbc0ea5571c12a895a79c64 
>   src/core-impl/collections/db/sql/SqlMeta.cpp a9b678aa4e3e55a2394da6f6581c159580fe6fc5 
>   src/core-impl/collections/db/sql/SqlRegistry.h 8d801178cc070c7772363d28d86ca63467996ebe 
>   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 6699b982e124193d068e2bd093bf0d15d6e34a9c 
> 
> 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/20120711/dfe43bbb/attachment.html>