Review Request: DB: change lyrics table: text url -> integer url pointing to the urls table

[Bart Cerneels]

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


I'm not all that familiar with lyrics & the SQL schema so will leave others to call it shipable.

Make sure to update HACKING/mysql_database_schema.txt when you commit.


ChangeLog
<http://git.reviewboard.kde.org/r/104966/#comment11106>

    "data are migrated" tickles my English grammar bone. "data is migrated" sounds better, even though it might not technically be correct with data plural.



ChangeLog
<http://git.reviewboard.kde.org/r/104966/#comment11105>

    Does this patch fix this? Or just makes it so rescanning is not required anymore?



src/core-impl/collections/db/sql/DatabaseUpdater.cpp
<http://git.reviewboard.kde.org/r/104966/#comment11107>

    You don't actually check if the previous query succeeded.



src/core-impl/collections/db/sql/DatabaseUpdater.cpp
<http://git.reviewboard.kde.org/r/104966/#comment11108>

    I was wondering if the url column could actually be intentional to point to a remote resource. But if it's not used currently there is no point in having it.


- Bart Cerneels


On May 16, 2012, 1:51 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104966/
> -----------------------------------------------------------
> 
> (Updated May 16, 2012, 1:51 p.m.)
> 
> 
> Review request for Amarok and Ralf Engels.
> 
> 
> Description
> -------
> 
> DB: change lyrics table: text url -> integer url pointing to the urls table
> 
> I believe that the old lyrics table structure was more or less a mistake:
> TABLE lyrics (
>     id INTEGER PRIMARY KEY, -- actually never used in code
>     url VARBINARY(324), -- actually a rpath from urls table
>     lyrics TEXT
> )
> 
> Apart from data duplication and non-conformity to the "update anomaly"
> requirement of the database design, there was additional problem that rpath
> itself is not unique. The (deviceId, rpath) is.
> 
> This change makes the lyrics table look more sane:
> TABLE lyrics (
>     url INTEGER PRIMARY KEY, -- points to url.id column
>     lyrics TEXT
> )
> 
> with an effort to transition existing entries. The transition of 5000
> lyrics entries takes 16s on my 2.5 GHz Core i5 (one core used), so it
> should be acceptable.
> 
> This is the first time I'm changing db structure, I'd be glad to
> receive thorough review, namely of the update13to14() method and
> especially the duplicate-removing query. This is critical because
> database-corrupting fault would leave many Amarok users in a state
> where they would need to drop their database to make Amarok working
> again.
> 
> Note to reporters of bug 242350: there's an unrelated bug 299150 which
> now applies to lyrics, too.
> 
> ChangeLog of the unrelated iPod fix is updated because DB_VERSION bump
> triggers full collection rescan as far as is documented.
> 
> BUG: 242350
> FIXED-IN: 2.6
> REVIEW: 104966
> 
> 
> This addresses bug 242350.
>     https://bugs.kde.org/show_bug.cgi?id=242350
> 
> 
> Diffs
> -----
> 
>   ChangeLog 67bc020 
>   src/core-impl/collections/db/sql/DatabaseUpdater.h 37ccb54 
>   src/core-impl/collections/db/sql/DatabaseUpdater.cpp 163b089 
>   src/core-impl/collections/db/sql/SqlMeta.cpp f8f9bdb 
> 
> Diff: http://git.reviewboard.kde.org/r/104966/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

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