Review Request: SqlScanResultProcessor: fix data-loss bug; squashed commits, recent on top
Ralf Engels
ralf-engels at gmx.de
Mon Jul 23 10:30:07 UTC 2012
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105488/#review16264
-----------------------------------------------------------
Ship it!
Provided the auto test run through and if you are really feeling brave enought you can push it.
- Ralf Engels
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/20120723/a13bd041/attachment.html>
More information about the Amarok-devel
mailing list