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

Bigint unsigned ids #7081

Merged
merged 3 commits into from Jan 14, 2019

Conversation

@escopecz
Copy link
Member

commented Jan 3, 2019

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? Y
New feature? N
Automated tests included? N
Related user documentation PR URL /
Related developer documentation PR URL /
Issues addressed (#s or URLs) /
BC breaks? N
Deprecations? N

Description:

We noticed that some of Mautic instances we manage has quite high autoincrements on statistical database tables already and we calculated that we have very few months until they overflow.

This PR changes all IDs to be INT UNSIGNED which will double the max INT limit. It also introduces new BIGINT UNSIGNED columns which are used on selected statistical tables.

I made a little research as I realized that PHP has limits on integers as well. It's defined by constant:

PHP_INT_MAX

The largest integer supported in this build of PHP. Usually int(2147483647) in 32 bit systems and int(9223372036854775807) in 64 bit systems. Available since PHP 5.0.5

Source: http://php.net/manual/en/reserved.constants.php

Note: 9223372036854775807 ~= 2^63

Mysql:

  • INT SIGNED max = 2147483647 < that's what we have now
  • BIGINT SIGNED max = 2^63
  • BIGINT UNSIGNED max = 2^64 < that's what we want

Source: https://dev.mysql.com/doc/refman/5.7/en/integer-types.html

So what we are using now is fine and will work on 32 bit processors for the whole range. If we switch to BIGINT SIGNED then 32 bit processors would work only to its limits. 64 bit processors the whole range. For BIGINT UNSIGNED even 64 bit processors won't be enough and we'll have to convert int to string which would cause troubles in many places within Mautic.

I consulted this with our team and we decided to switch to BIGINT UNSIGNED anyway as we should not get over 2^63. And users running Mautic on 32 bit systems should not get to its limits too as we suppose those will be small instances.

This changes will appear on new installations only. This PR does not contain migrations of current tables on purpose. Those migrations would be massive as MySql will create a copy of the current stat tables to alter the column type. This can lead to big troubles during migrations of big-size databases such as run out of disk space or overloading database with incoming traffic while the tables are locked.

Not only ID columns will be affected by this change. Doctrine is more clever. It changes types also to all foreign key columns. So when I changed leads.id to BIGINT then all tables containing lead_id column will be re-typed too. So it will affect majority of the tables anyway.

If your instance is getting close to the INT limit then follow these steps to migrate:

  1. Find a time with low incoming traffic when to do this migration.
  2. Consider blocking the traffic temporarily on the server level.
  3. Generate the migration SQL queries with app/console doctrine:schema:update --dump-sql | grep BIGINT
  4. Execute the queries. One by one would be safer.
  5. Enable the incoming traffic again.

Steps to reproduce the bug:

  1. Theoretically, generate over 2147483647 rows on some table. But there is no need to replicate this.

Steps to test this PR:

  1. If you checkout this PR to your existing Mautic instance it will do nothing.
  2. If you checkout this PR and install new Mautic from it then the statistical tables will have ID column type of BIGINT UNSIGNED and all other tables INT UNSIGNED.
Woeler and others added 3 commits Dec 19, 2018

@escopecz escopecz added this to the 2.15.1 milestone Jan 3, 2019

@npracht

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

Question @escopecz. This is very great (also #7082), but both include a migration file. Didn't we say we keep everything touching the DB schema for minor versions (then it would be for 2.16.0)?

@escopecz

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

This PR has no migration. I'm not aware of such decision, but the release leader can decide about the faith of each PR. I have no problem moving it to 2.16.0.

@npracht

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

Yeah i meant it because of:

This PR does not contain migrations of current tables on purpose. Those migrations would be massive as MySql will create a copy of the current stat tables to alter the column type. This can lead to big troubles during migrations of big-size databases such as run out of disk space or overloading database with incoming traffic while the tables are locked.

I thought we talked about it. But I feel confortable giving this responsibility to the release leader :D

@escopecz

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

Yep, that's the reason this PR does not contain migration and it will be applicable only for new installations. The migration process for old installations is described there as well if someone needs it now.

@mleffler

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

Tested and works properly, also code reviewed and running in our production environment. 2.16 or whatever folks decide is fine.

@npracht npracht added this to Ready to Test (confirmation) in Mautic 2 Jan 3, 2019

@alanhartless alanhartless added this to Needs Second Test in 2.15.1 Jan 14, 2019

Mautic 2 automation moved this from Ready to Test (confirmation) to Ready to Test (first time) Jan 14, 2019

@alanhartless alanhartless merged commit cd94443 into mautic:staging Jan 14, 2019

2 checks passed

Scrutinizer Analysis: 3 new issues, 1 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Mautic 2 automation moved this from Ready to Test (first time) to Merged Jan 14, 2019

@alanhartless alanhartless moved this from Needs Second Test to Merged in 2.15.1 Jan 15, 2019

@kuzmany

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

If I call app/console doctrine:schema:update --dump-sql
I got a lot of errors with this PR. Are you shure it's not BC?

  [Doctrine\DBAL\Exception\DriverException]
  An exception occurred while executing 'ALTER TABLE oauth1_access_tokens CHANGE consumer_id consumer_id INT UNSIGNED NOT NULL, CHANGE user_id user_id INT UNSIGNED
   NOT NULL':

  SQLSTATE[HY000]: General error: 1832 Cannot change column 'consumer_id': used in a foreign key constraint 'FK_C33AC86237FDBD6D'

  [Doctrine\DBAL\Driver\PDOException]
  SQLSTATE[HY000]: General error: 1832 Cannot change column 'consumer_id': used in a foreign key constraint 'FK_C33AC86237FDBD6D'

  [PDOException]
  SQLSTATE[HY000]: General error: 1832 Cannot change column 'consumer_id': used in a foreign key constraint 'FK_C33AC86237FDBD6D'
@alanhartless

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

@escopecz was able to reproduce so we'll revert. It was meant for new installations to prepare for large amount of data. But a migration is ginormous so opted not to do it.

@alanhartless alanhartless deleted the mautic-inc:bigint-unsigned-ids branch Apr 18, 2019

@heathdutton

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Is there a way to modify the schema for new installations without causing migrations for existing ones?

@escopecz

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

The schema for new installations is generated from the Doctrine entities. In theory, users shouldn't need anything else to migrate from one version to another than the app/console doctrine:migrations:migrate command. So again, in theory, we can update a database column type in a Doctrine entity and purposefully ignore this change in the migrations. This way the new installations will have that new type and old will live with the old one.

That's what I was aiming for with this PR.

Sadly, in practice we sometimes need the app/console doctrine:schema:update --force which did not work with these changes as @kuzmany discovered above. And that's probably the reason this PR was reverted.

@heathdutton

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

Ok, but I think the issue @kuzmany noted was not during --dump-sql but during execution of that SQL. When running such alterations you must run SET FOREIGN_KEY_CHECKS=0; first. The doctrine dump-sql will not output this for you.

Edit: I stand corrected, apparently the order of the alter statements also needs to be more specific than DBAL understands. Hmm.

@escopecz

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Good point. It's with the --force flag when the SQL queries get executed. That's probably what he meant. Would you want to give this PR another chance?

@heathdutton

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

I'd like to, but there is no conceivable way to make the doctrine:schema:update work with data type changes to foreign keys.

You have to to manually remove those foreign key constraints, then run the SQL, then recreate those foreign key constraints. Otherwise you get errors like this:

Error in foreign key constraint of table mautictest/asset_downloads:
there is no index in the table which would contain
the columns as the first columns, or the data types in the
table do not match the ones in the referenced table
or one of the ON ... SET NULL columns is declared NOT NULL. Constraint:
,
  CONSTRAINT `FK_A6494C8F55458D` FOREIGN KEY (`lead_id`) REFERENCES `leads` (`id`) ON DELETE SET NULL

I'm running out of ideas on this :(

@heathdutton

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Can we also define custom FK names, so that those old ones are removed, and then recreated?

@escopecz

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

We have a method somewhere that generates the FK names the way Doctrine does so the doctrine:schema:update command wouldn't want to drop them and recreate with different name. It takes the table name, column names and table prefix and builds the hash out of it. Because of the table prefix it can have different names for different Mautic installations.

I plan to make this change for M3 which may be the best opportunity for this. Users may expect more tedious migration coming from M2 to M3.

@heathdutton

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

That makes the most sense. I'll abandon my PR and continue to make these migrations manually for now. Please also include the ip_addresses table ID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
7 participants
You can’t perform that action at this time.