Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Universal DB upgrade-via-copy support for sqlite and mysql. #467

Merged
merged 4 commits into from Oct 18, 2011

Conversation

firnsy
Copy link
Contributor

@firnsy firnsy commented Oct 5, 2011

In summary this PR addresses the following:

  1. aligns the DB upgrade procedure with sqlite
  2. aligns music and db upgrade methods
  3. minor cosmetics

Firstly, the DB upgrade procedure for sqlite was to copy the old versioned DB file to the new version DB file and then perform the upgrade on the new version DB file only. The benefit, if not obvious, was that the data was preserved in the event of a failure. This patchset now provides the same functionality for mysql users by creating a new db based on the old version schema and then performing the upgrade on the new DB only. What this does mean that the mysql credentials sufficient to "CREATE DATABASE".

Secondly, the music DB was not using a transaction for the db upgrade method, it was possible on sqlite systems that it could bail half way through an upgrade and leave it in an unknown state. Not so much an issue with the upgrade-via-copy capability, but it aligns with the video DB upgrade method.

Regards,
firnsy

@Montellese
Copy link
Member

I didn't take a very close look, the only thing I saw is the "global" call to CreateViews() at the end of every db update independant of what changes. The way it is now there may be multiple calls to CreateViews() during a full database update. Do we want this? Shouldn't a single call to CreateViews() at the end of the whole update procedure be enought?

@firnsy
Copy link
Contributor Author

firnsy commented Oct 5, 2011

Yes it's possible that multiple views can be created in a DB update. CreateViews will drop the view before recreating it so it won't cause any significant issues.

If the multiple occurrences are an issue for pull, then I'll remove them in a separate commit and append it to this PR.

@Montellese
Copy link
Member

I know that it won't do any harm just thought it might safe some work for the DBMS but it's not a blocker at all.

@CrystalP
Copy link
Contributor

CrystalP commented Oct 5, 2011

Are indexes copied with the CREATE TABLE LIKE statements? (mysql newb here)

@firnsy
Copy link
Contributor Author

firnsy commented Oct 5, 2011

@CrystalP, yes they are.

@bossanova808
Copy link
Contributor

I've collected various MySQL optimisations from the forum, not sure if they are worth integrating in this patch, (and please excuse the butting in of a total non dev). If they were built in so to speak, there would be less annoying 'MySQL is running slow' posts :)

From my notes:

In advancedsettings.xml:
Use the IP address and NOT the hostname of your MySQL server

In my.ini:
(add to mysqld section)
skip-name-resolve

At dos cmd:
mysqlcheck -op -u xbmc xbmc_video
mysqlcheck -op -u xbmc xbmc_music

In MySQL command tool:
use xbmc_music;
CREATE INDEX idAlbum_idx ON song(idAlbum);
CREATE INDEX idArtist_idx ON song(idArtist);
CREATE INDEX idArtist_idx ON album(idArtist);
CREATE INDEX idArtist_idx ON exartistsong(idArtist);
CREATE INDEX idArtist_idx ON exartistalbum(idArtist);
ALTER TABLE xbmc_music.song ADD INDEX idx_idArtist(idArtist);
ALTER TABLE xbmc_music.song ADD INDEX idx_idGenre(idGenre);
ALTER TABLE xbmc_music.song ADD INDEX idx_idAlbum(idAlbum);

use xbmc_video;
ALTER TABLE movie ADD INDEX idMovie(idMovie);
ALTER TABLE movie ADD INDEX idFile(idFile);

@garbear
Copy link
Member

garbear commented Oct 6, 2011

and please excuse the butting in of a total non dev

Correction, bossanova, you're on a dev site, participating in dev things, AND you're bringing valuable research to the table. Ipso facto, youre a dev as much as any of us. [ok, back to dev things]

@bossanova808
Copy link
Contributor

Well, cheers! Just figure if the indexes in particular cause no harm, they might as well be in there - makes a huge difference with the music database especially, I have a largeish collection of 1500 odd albums etc, and without the indexes its a bit painful. I'd be happy to chuck MySQL in a way, but sqllite is not great for multi-user so even with Eden's new path substitution etc, seems MySQL is still the way to go.

@firnsy
Copy link
Contributor Author

firnsy commented Oct 6, 2011

@bossanova808,

The 6 indexes highlighted for the music table were added in a961454.

As for the video table indexes, I'll put them on my TODO.

@jmarshallnz
Copy link
Contributor

Overall looks good as well as it's very well tested.

@bossanova808
Copy link
Contributor

So presumably this will mean the databases move to a standardised name and the name bit in advanced settings will be ignored once the upgrade is done? Just like sqllite.

@firnsy
Copy link
Contributor Author

firnsy commented Oct 11, 2011

Small fix and rebase to remove server side include header that hurts osx/ios builds.

@jmarshallnz, I've tested the upgrade on both sqlite and mysql instances from dharma to current head with no issues thus far. I will however await amet's findings on atv2/ios. There still exists a potential issue for Dharma db's that have unicode data in a non-unicode collated db which will cause the new db to also be created with non-unicode collation. XBMC now builds any new mysql tables with unicode collation explictly. Those scenarios will require exporting the old database to NFO and then reimporting on the new. This patch set however does assure that for those upgrading from dharma won't have their mysql db obliterated by upgrading the original tables.

@bossanova808, The name override should still be possible but just not a mandatory field anymore in as.xml

@amet
Copy link
Contributor

amet commented Oct 16, 2011

@firnsy you need to address @jmarshallnz if you expect the answer :)

still, we are not checking for the existing DB before we create the new one with appended version? what happens if it exist, is it just ignored?

@firnsy
Copy link
Contributor Author

firnsy commented Oct 17, 2011

@amet, yes it will, eventually ;)

This is rebased with recent head.

firnsy pushed a commit that referenced this pull request Oct 18, 2011
Universal DB upgrade-via-copy support for sqlite and mysql.
@firnsy firnsy merged commit 9c72e40 into xbmc:master Oct 18, 2011
@ghost
Copy link

ghost commented Oct 19, 2011

@firnsy please contact 'wonslung' in #xbmc - he has update issues.

@Zxurian
Copy link

Zxurian commented Oct 20, 2011

I'm thinking that this commit is what has my mysql install not working. Just upgraded to Oct 19th nightly, and xmbc goes into a crash loop when using mysql settings under advancsettings.xml (xmbc-live install)

log here: http://pastebin.com/mCkH4six - this log was generated after I tried a blank install of the Oct19th nightly, was working, then went back and added the mysql settings into advancedsettings.xml, upon reboot, that log is the result

@Albinoman887
Copy link

same issue as above poster using mysql. havent tryed sqlite is this what the local dbs are based on? anyways back on tpic have the same loop as above poster after this commit

@Albinoman887
Copy link

i tryed reverting this commit but it still didnt fix the issue although i might not have reverted correctly i simply did git reset

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants