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

Command line tool to convert current database to others, except sqlite #6457

Merged
merged 52 commits into from May 19, 2014

Conversation

Projects
None yet
@bartv2
Member

bartv2 commented Dec 16, 2013

I think it needs some more help/description what it does, but can't think of what to write.

@karlitschek @butonic @DeepDiver1975 @bantu @ringmaster

@ringmaster

This comment has been minimized.

Show comment
Hide comment
@ringmaster

ringmaster Dec 16, 2013

Contributor

Any chance this could be made generic to move between any database engines?

Contributor

ringmaster commented Dec 16, 2013

Any chance this could be made generic to move between any database engines?

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Dec 17, 2013

Member

Great. @bartv2 Do you think this could be extended to move between any databases?

Member

karlitschek commented Dec 17, 2013

Great. @bartv2 Do you think this could be extended to move between any databases?

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Dec 18, 2013

Member

Please make use of dependency injection. The quality of the code in master needs to improve.

Member

bantu commented Dec 18, 2013

Please make use of dependency injection. The quality of the code in master needs to improve.

@kprovost

This comment has been minimized.

Show comment
Hide comment
@kprovost

kprovost Jan 4, 2014

Contributor

I've been trying to use this code to convert from SQLite to postgreSQL and I've run into a couple of issues.

The biggest one is that the owncloud db uses the 'user' field all over the place and that's a reserved keyword. As a result it needs to be escaped during insert calls, which this code doesn't do. I've hacked around it by adding the escapes in the DBAL layer, but that's probably not the cleanest solution.

I've also had trouble with the size of the mime field in oc_files_trash and the value field in oc_contacts_cards_properties.

I added the following hacks just before the copyTable() foreach:

$toDB->exec("SET CONSTRAINTS ALL DEFERRED;");
$toDB->exec("ALTER TABLE oc_files_trash ALTER mime TYPE character varying(128);");
$toDB->exec("ALTER TABLE oc_contacts_cards_properties ALTER value TYPE text;");
$toDB->exec("DROP INDEX cp_value_index");

Again, these are hacks, but at least they show where the problems lie.

Contributor

kprovost commented Jan 4, 2014

I've been trying to use this code to convert from SQLite to postgreSQL and I've run into a couple of issues.

The biggest one is that the owncloud db uses the 'user' field all over the place and that's a reserved keyword. As a result it needs to be escaped during insert calls, which this code doesn't do. I've hacked around it by adding the escapes in the DBAL layer, but that's probably not the cleanest solution.

I've also had trouble with the size of the mime field in oc_files_trash and the value field in oc_contacts_cards_properties.

I added the following hacks just before the copyTable() foreach:

$toDB->exec("SET CONSTRAINTS ALL DEFERRED;");
$toDB->exec("ALTER TABLE oc_files_trash ALTER mime TYPE character varying(128);");
$toDB->exec("ALTER TABLE oc_contacts_cards_properties ALTER value TYPE text;");
$toDB->exec("DROP INDEX cp_value_index");

Again, these are hacks, but at least they show where the problems lie.

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Jan 5, 2014

Member

@bartv2 So what is the status here?

Member

karlitschek commented Jan 5, 2014

@bartv2 So what is the status here?

@bartv2

This comment has been minimized.

Show comment
Hide comment
@bartv2

bartv2 Jan 5, 2014

Member

@kprovost thanks for testing this, i only checked sqlite->mysql, where this apparently wasn't a problem.
@karlitschek still working on this, just been distracted the last couple of weeks

Member

bartv2 commented Jan 5, 2014

@kprovost thanks for testing this, i only checked sqlite->mysql, where this apparently wasn't a problem.
@karlitschek still working on this, just been distracted the last couple of weeks

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek Jan 5, 2014

Member

Cool. Thanks.

Member

karlitschek commented Jan 5, 2014

Cool. Thanks.

@kprovost

This comment has been minimized.

Show comment
Hide comment
@kprovost

kprovost Jan 7, 2014

Contributor

Just ran into another problem: the sequences need to be altered to start from max(id) + 1.

For those unfamiliar with postgres, sequences generate the next ID for index (or any other column you like) columns, much like the MySQL auto_increment. Apparently the conversion left me in a situation where the sequence generated IDs which already existed.

I (manually) fixed it by just doing select max(id) from ...; and then alter sequence ... restart with [max(id) + 1];

Contributor

kprovost commented Jan 7, 2014

Just ran into another problem: the sequences need to be altered to start from max(id) + 1.

For those unfamiliar with postgres, sequences generate the next ID for index (or any other column you like) columns, much like the MySQL auto_increment. Apparently the conversion left me in a situation where the sequence generated IDs which already existed.

I (manually) fixed it by just doing select max(id) from ...; and then alter sequence ... restart with [max(id) + 1];

@bartv2

This comment has been minimized.

Show comment
Hide comment
@bartv2

bartv2 Jan 17, 2014

Member

It still only converts from sqlite, next is making it work to also convert from other database types

Member

bartv2 commented Jan 17, 2014

It still only converts from sqlite, next is making it work to also convert from other database types

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Jan 17, 2014

Member

Is this something you want to try in this PR?

Member

bantu commented Jan 17, 2014

Is this something you want to try in this PR?

@bartv2

This comment has been minimized.

Show comment
Hide comment
@bartv2

bartv2 Jan 17, 2014

Member

Is this something you want to try in this PR?

No lets do that in another PR. That should also make this easier to test.

Member

bartv2 commented Jan 17, 2014

Is this something you want to try in this PR?

No lets do that in another PR. That should also make this easier to test.

@bartv2

This comment has been minimized.

Show comment
Hide comment
@bartv2

bartv2 Jan 31, 2014

Member

Any more comments on this? Or can we merge this?

Member

bartv2 commented Jan 31, 2014

Any more comments on this? Or can we merge this?

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jan 31, 2014

Member

No lets do that in another PR. That should also make this easier to test.

I'd prefer to do it all at once and right - partial solution would help that much from my point of view.
In addition there is no rush in get this merged now (besides the fact that the branch was kind of rotting the past weeks 😉 )

Member

DeepDiver1975 commented Jan 31, 2014

No lets do that in another PR. That should also make this easier to test.

I'd prefer to do it all at once and right - partial solution would help that much from my point of view.
In addition there is no rush in get this merged now (besides the fact that the branch was kind of rotting the past weeks 😉 )

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier May 12, 2014

The inspection completed: 8 new issues, 22 updated code elements

scrutinizer-notifier commented May 12, 2014

The inspection completed: 8 new issues, 22 updated code elements

@owncloud-bot

This comment has been minimized.

Show comment
Hide comment
@owncloud-bot

owncloud-bot May 12, 2014

Contributor

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4735/

Contributor

owncloud-bot commented May 12, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4735/

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek May 14, 2014

Member

@Nowaker Can you help to review that?

Member

karlitschek commented May 14, 2014

@Nowaker Can you help to review that?

@Nowaker

This comment has been minimized.

Show comment
Hide comment
@Nowaker

Nowaker May 14, 2014

@karlitschek I don't think so. I use version 6.

Nowaker commented May 14, 2014

@karlitschek I don't think so. I use version 6.

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu May 19, 2014

Member

@karlitschek Consider merging without additional review.

Member

bantu commented May 19, 2014

@karlitschek Consider merging without additional review.

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek May 19, 2014

Member

@schiesbn @PVince81 @DeepDiver1975 Can anyone have a quick look?

Member

karlitschek commented May 19, 2014

@schiesbn @PVince81 @DeepDiver1975 Can anyone have a quick look?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 19, 2014

Member

The code looks good, didn't test.

Would it make sense to automatically enable maintenance mode before the conversion starts and disable it afterwards ?

Member

PVince81 commented May 19, 2014

The code looks good, didn't test.

Would it make sense to automatically enable maintenance mode before the conversion starts and disable it afterwards ?

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 May 19, 2014

Member

I did review this PR last week already - without testing.

I'm fine with the code as well

Member

DeepDiver1975 commented May 19, 2014

I did review this PR last week already - without testing.

I'm fine with the code as well

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu May 19, 2014

Member

Would it make sense to automatically enable maintenance mode before the conversion starts and disable it afterwards ?

This is being done here. https://github.com/owncloud/core/pull/6457/files#diff-edd78618a1329a6cad8cfffd51901e60R259

Member

bantu commented May 19, 2014

Would it make sense to automatically enable maintenance mode before the conversion starts and disable it afterwards ?

This is being done here. https://github.com/owncloud/core/pull/6457/files#diff-edd78618a1329a6cad8cfffd51901e60R259

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 19, 2014

Member

@bantu ok got it. Seems I used the search on the wrong page.
👍

Member

PVince81 commented May 19, 2014

@bantu ok got it. Seems I used the search on the wrong page.
👍

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu May 19, 2014

Member

@karlitschek Let's have this then?

Member

bantu commented May 19, 2014

@karlitschek Let's have this then?

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek
Member

karlitschek commented May 19, 2014

👍

karlitschek pushed a commit that referenced this pull request May 19, 2014

Frank Karlitschek
Merge pull request #6457 from owncloud/db-convert-tool
Command line tool to convert current database to others, except sqlite

@karlitschek karlitschek merged commit 090d127 into master May 19, 2014

1 check passed

default Merged build finished.
Details

@karlitschek karlitschek deleted the db-convert-tool branch May 19, 2014

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek May 19, 2014

Member

done :-)

Member

karlitschek commented May 19, 2014

done :-)

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu
Member

bantu commented May 19, 2014

@karlitschek Merci

@cdamken

This comment has been minimized.

Show comment
Hide comment
@cdamken

cdamken May 21, 2014

Contributor

@karlitschek

It seems this is completed? Has it been tested? I have a need for this script at the moment. If it is ready for release, are there instructions on how to use?

Contributor

cdamken commented May 21, 2014

@karlitschek

It seems this is completed? Has it been tested? I have a need for this script at the moment. If it is ready for release, are there instructions on how to use?

@karlitschek

This comment has been minimized.

Show comment
Hide comment
@karlitschek

karlitschek May 22, 2014

Member

@cdamken It is now merged and should work. Please not that this is not a simple conversion script but requires complex changes in core. So this works only in ownCloud 7 and is not available in older versions.

Member

karlitschek commented May 22, 2014

@cdamken It is now merged and should work. Please not that this is not a simple conversion script but requires complex changes in core. So this works only in ownCloud 7 and is not available in older versions.

@Nowaker

This comment has been minimized.

Show comment
Hide comment
@Nowaker

Nowaker May 22, 2014

Will there be owncloud 6 to 7 migration script, so I could then convert my SQLite db to PostgreSQL with your converter?

Nowaker commented May 22, 2014

Will there be owncloud 6 to 7 migration script, so I could then convert my SQLite db to PostgreSQL with your converter?

@holmboe

This comment has been minimized.

Show comment
Hide comment
@holmboe

holmboe May 28, 2014

I am also in need of this. I have a server on it's knees. :)

My current setup is an OC v6 with sqlite3 so an update to OC v7, when it is released, before migrating mysql is totally fine.

holmboe commented May 28, 2014

I am also in need of this. I have a server on it's knees. :)

My current setup is an OC v6 with sqlite3 so an update to OC v7, when it is released, before migrating mysql is totally fine.

@Bugsbane

This comment has been minimized.

Show comment
Hide comment
@Bugsbane

Bugsbane Aug 20, 2014

"It is now merged and should work. Please not that this is not a simple conversion script but requires complex changes in core. So this works only in ownCloud 7 and is not available in older versions."

@karlitschek - So for someone trying to migrate a v7.0.1 database, how do we go about it? I'm moving my sqlite3 / v7.0.1 instance to a server that only supports Mysql currently, and am keen to not accidentally use one of those DB breaking proceedures that seemed popular previously.

Bugsbane commented Aug 20, 2014

"It is now merged and should work. Please not that this is not a simple conversion script but requires complex changes in core. So this works only in ownCloud 7 and is not available in older versions."

@karlitschek - So for someone trying to migrate a v7.0.1 database, how do we go about it? I'm moving my sqlite3 / v7.0.1 instance to a server that only supports Mysql currently, and am keen to not accidentally use one of those DB breaking proceedures that seemed popular previously.

@HeisSpiter

This comment has been minimized.

Show comment
Hide comment
@HeisSpiter

HeisSpiter Aug 21, 2014

@Bugsbane Basically, you've to use OwnCloud Console (occ). It's in root directory for your owncloud installation. If you installed it in /var/ww/owncloud, it's /var/www/owncloud/occ. So, basically, you need to:

/var/www/owncloud/occ db:convert-type

For usage, see:

/var/www/owncloud/occ help db:convert-type

HeisSpiter commented Aug 21, 2014

@Bugsbane Basically, you've to use OwnCloud Console (occ). It's in root directory for your owncloud installation. If you installed it in /var/ww/owncloud, it's /var/www/owncloud/occ. So, basically, you need to:

/var/www/owncloud/occ db:convert-type

For usage, see:

/var/www/owncloud/occ help db:convert-type
@Bugsbane

This comment has been minimized.

Show comment
Hide comment
@Bugsbane

Bugsbane Aug 21, 2014

Great. Thanks HeisSpiter. That's exactly what I needed!

Bugsbane commented Aug 21, 2014

Great. Thanks HeisSpiter. That's exactly what I needed!

@ghost ghost referenced this pull request Apr 18, 2016

Closed

Error converting DB type to Postgres #24065

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment