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

Create upgrade_column_info_4_6_5+.sql #12676

Closed
wants to merge 2 commits into from
Closed

Create upgrade_column_info_4_6_5+.sql #12676

wants to merge 2 commits into from

Conversation

danielgp
Copy link
Contributor

@danielgp danielgp commented Nov 1, 2016

Before submitting pull request, please check that every commit:

  • [x ] Has proper Signed-Off-By
  • [x ] Has commit message which describes it
  • [x ] Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • [x ] Any new functionality is covered by tests

Bookmark id that is an auto_increment column can only take positive values starting from 0, therefore should be unsigned otherwise half of allowed interval is wasted from start, and this is the fix for that

Signed-off-by: Daniel Popiniuc danielpopiniuc@gmail.com

Bookmark id that is an auto_increment colunm can only take positive values starting from 0, therefore should be unsigned otherwise half of allowed interval is waisted from start, and this is the fix for that
@codecov-io
Copy link

codecov-io commented Nov 1, 2016

Current coverage is 50.18% (diff: 100%)

Merging #12676 into QA_4_6 will not change coverage

@@             QA_4_6     #12676   diff @@
==========================================
  Files           485        485          
  Lines         81344      81344          
  Methods        2126       2126          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          40821      40821          
  Misses        40523      40523          
  Partials          0          0          

Powered by Codecov. Last update 8556b5d...732735d

@danielgp
Copy link
Contributor Author

danielgp commented Nov 2, 2016

#12676 I have added the Sign-Off-By line and now everything should be ok

…alues starting from 0, therefore should be unsigned otherwise half of allowed interval is waisted from start, and this is the fix for that
@nijel
Copy link
Contributor

nijel commented Nov 8, 2016

Why this change? Is int limiting number of your bookmarks?

@nijel nijel self-assigned this Nov 8, 2016
@danielgp
Copy link
Contributor Author

danielgp commented Nov 8, 2016

@nijel
This change is meant to bring consistency for all "auto_increment" fields in the database which do have the "unassigned" flag except the bookmark one.
Having the "unassigned" flag for an "auto_increment" field is quite normal as the first value will be 1 which is giving a full use of the interval, without it 50% of the interval will be wasted right from the start, see https://dev.mysql.com/doc/refman/5.7/en/integer-types.html .
In conclusion even if the limitation of int(11) without unassigned is quite high, to 2+ billion records, it's more about consistency with all the other "auto_increment" fields, and avoid wasting 50% of the interval right from the start than bringing the limit to 4+ billion.

@nijel
Copy link
Contributor

nijel commented Nov 8, 2016

Okay, as this is rather cleanup than bug fix, it should rather go to master branch instead of QA_4_6.

@danielgp
Copy link
Contributor Author

danielgp commented Nov 8, 2016

@nijel I have modified this merge request against master as instructed, thanks!

@danielgp danielgp changed the base branch from QA_4_6 to master November 8, 2016 20:34
@nijel
Copy link
Contributor

nijel commented Nov 9, 2016

Yes, please rebase your changes on master and open PR for that.

@nijel nijel closed this Nov 9, 2016
@danielgp danielgp deleted the patch-1 branch November 9, 2016 12:55
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

3 participants