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

Migrate ID columns to bigint #6563

Merged
merged 8 commits into from
Sep 29, 2017
Merged

Migrate ID columns to bigint #6563

merged 8 commits into from
Sep 29, 2017

Conversation

nickvergessen
Copy link
Member

For better scaling

@mention-bot
Copy link

@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rullzer, @MorrisJobke and @LukasReschke to be potential reviewers.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me

@codecov
Copy link

codecov bot commented Sep 19, 2017

Codecov Report

Merging #6563 into master will increase coverage by 3.19%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #6563      +/-   ##
============================================
+ Coverage     53.06%   56.25%   +3.19%     
============================================
  Files          1417       61    -1356     
  Lines         87809     7574   -80235     
  Branches       1340     1340              
============================================
- Hits          46596     4261   -42335     
+ Misses        41213     3313   -37900
Impacted Files Coverage Δ Complexity Δ
apps/files_external/lib/Command/Delete.php
settings/ajax/setquota.php
apps/files_external/lib/Lib/Backend/DAV.php
apps/theming/lib/ThemingDefaults.php
lib/private/Preview/Image.php
apps/dav/lib/CardDAV/Converter.php
lib/private/Files/Cache/StorageGlobal.php
...s/workflowengine/lib/Controller/FlowOperations.php
core/Command/Maintenance/UpdateTheme.php
apps/workflowengine/lib/Check/RequestTime.php
... and 1345 more

Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflict

@nickvergessen
Copy link
Member Author

nickvergessen commented Sep 19, 2017

Yeah, as to be expected. I will rebase after #6564 and #6566

@nickvergessen
Copy link
Member Author

Rebased, ready for review and merge

@nickvergessen nickvergessen force-pushed the bigint-ids branch 2 times, most recently from 4ef49be to beea34b Compare September 20, 2017 07:16
@nickvergessen
Copy link
Member Author

@LukasReschke please review, so this is available early in dailies and master for testing

/**
* Auto-generated migration step: Please modify to your needs!
*/
class Version1004Date20170919103422 extends BigIntMigration {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not executed at all. :/

My migrations table for dav looks like this:


dav | 1004Date20170825134824
dav | 1004Date20170919104507
dav | 1004Date20170924124212

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I bet that is ebcause we merged the other migration to dav.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah need to rebase and change the timestamp to be newer...
#JustDevProblems....

Will have a look separate if we can detect not executed migrations and run them individually

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will have a look separate if we can detect not executed migrations and run them individually

isn't that the idea of migrations? xD I thought it will just be checked what was already done and what not and then execute all not executed ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No the issue is you need to do it sequentially. So you can't just insert a new one in the middle. Because that could lead to inconsistencies.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, I would also only allow this for debug,...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay actually it should have worked, the problem is another PR bumped the version number for the dav app on master already, therefor no update was done at all.

Fixed it now.

@nickvergessen nickvergessen force-pushed the bigint-ids branch 2 times, most recently from 53d0734 to 4243f40 Compare September 26, 2017 14:14
return [
'addressbooks' => ['id'],
'addressbookchanges' => ['id', 'addressbookid'],
'calendars' => ['id', 'calendarid'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doctrine\DBAL\Schema\SchemaException: There is no column with name 'calendarid' on table 'oc_calendars'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stupid copy and paste, but I checked the dav tables and it looks good now.

@codecov-io
Copy link

codecov-io commented Sep 27, 2017

Codecov Report

Merging #6563 into master will decrease coverage by 0.01%.
The diff coverage is 34.14%.

@@             Coverage Diff              @@
##             master    #6563      +/-   ##
============================================
- Coverage     53.03%   53.01%   -0.02%     
- Complexity    22583    22606      +23     
============================================
  Files          1417     1422       +5     
  Lines         87873    87954      +81     
  Branches       1341     1341              
============================================
+ Hits          46601    46629      +28     
- Misses        41272    41325      +53
Impacted Files Coverage Δ Complexity Δ
...av/lib/Migration/Version1004Date20170926103422.php 0% <0%> (ø) 1 <1> (?)
version.php 0% <0%> (ø) 0 <0> (ø) ⬇️
...es/lib/Migration/Version1002Date20170926101419.php 0% <0%> (ø) 1 <1> (?)
core/Migrations/Version13000Date20170926101637.php 0% <0%> (ø) 1 <1> (?)
lib/public/Migration/BigIntMigration.php 0% <0%> (ø) 4 <4> (?)
lib/private/DB/ConnectionFactory.php 58.02% <33.33%> (-0.95%) 24 <0> (+1)
lib/private/DB/OCPostgreSqlPlatform.php 96.42% <96.42%> (ø) 15 <15> (?)
... and 2 more

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works 👍

@MorrisJobke
Copy link
Member

I also fixed the autoloader.

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 27, 2017
@nickvergessen
Copy link
Member Author

What, postgres fails?

Error while trying to create admin user: An exception occurred while executing 'ALTER TABLE oc_personal_settings ALTER id TYPE BIGSERIAL':

SQLSTATE[42704]: Undefined object: 7 ERROR: type "bigserial" does not exist

@nickvergessen
Copy link
Member Author

Copied owncloud/core#28364

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still working fine 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants