-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: migrations to make postgresql compatible. #35762
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
base: master
Are you sure you want to change the base?
Changes from 22 commits
70bbd68
c222853
fe65206
2335e69
4fd6ca0
1c4295c
60c4830
0a9cd28
00b235a
2614432
8c21658
d096773
63875d9
78c2209
fc80ce2
c739e41
c0f0695
03b4b00
43938ac
1f5c9d0
499c852
558f5ad
22dc5f9
139b8a6
cdd8403
42f9b3c
5e73119
023e5b0
30811ef
6e11456
c44cd5d
d4049f3
7df2ae2
aaa8a71
c4f859c
edcad98
c1c287c
f48ad00
41555c9
4ac2812
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,36 +1,3 @@ | ||
| """ | ||
| Custom fields | ||
| """ | ||
|
|
||
|
|
||
| from django.db.models.fields import AutoField | ||
|
|
||
|
|
||
| class UnsignedBigIntAutoField(AutoField): | ||
| """ | ||
| An unsigned 8-byte integer for auto-incrementing primary keys. | ||
| """ | ||
| def db_type(self, connection): | ||
| if connection.settings_dict['ENGINE'] == 'django.db.backends.mysql': | ||
| return "bigint UNSIGNED AUTO_INCREMENT" | ||
| elif connection.settings_dict['ENGINE'] == 'django.db.backends.sqlite3': | ||
| # Sqlite will only auto-increment the ROWID column. Any INTEGER PRIMARY KEY column | ||
| # is an alias for that (https://www.sqlite.org/autoinc.html). An unsigned integer | ||
| # isn't an alias for ROWID, so we have to give up on the unsigned part. | ||
| return "integer" | ||
| elif connection.settings_dict['ENGINE'] == 'django.db.backends.postgresql_psycopg2': | ||
| # Pg's bigserial is implicitly unsigned (doesn't allow negative numbers) and | ||
| # goes 1-9.2x10^18 | ||
| return "BIGSERIAL" | ||
| else: | ||
| return None | ||
|
|
||
| def rel_db_type(self, connection): | ||
| if connection.settings_dict['ENGINE'] == 'django.db.backends.mysql': | ||
| return "bigint UNSIGNED" | ||
| elif connection.settings_dict['ENGINE'] == 'django.db.backends.sqlite3': | ||
| return "integer" | ||
| elif connection.settings_dict['ENGINE'] == 'django.db.backends.postgresql_psycopg2': | ||
| return "BIGSERIAL" | ||
| else: | ||
| return None |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,6 @@ | |
| from opaque_keys.edx.keys import CourseKey, UsageKey | ||
| from simple_history.models import HistoricalRecords | ||
|
|
||
| from lms.djangoapps.courseware.fields import UnsignedBigIntAutoField | ||
| from lms.djangoapps.grades import events # lint-amnesty, pylint: disable=unused-import | ||
| from openedx.core.lib.cache_utils import get_cache | ||
| from lms.djangoapps.grades.signals.signals import ( | ||
|
|
@@ -327,7 +326,7 @@ class Meta: | |
| ] | ||
|
|
||
| # primary key will need to be large for this table | ||
| id = UnsignedBigIntAutoField(primary_key=True) # pylint: disable=invalid-name | ||
| id = models.BigAutoField(primary_key=True) # pylint: disable=invalid-name | ||
|
||
|
|
||
| user_id = models.IntegerField(blank=False) | ||
| course_id = CourseKeyField(blank=False, max_length=255) | ||
|
|
@@ -581,7 +580,7 @@ class Meta: | |
| ] | ||
|
|
||
| # primary key will need to be large for this table | ||
| id = UnsignedBigIntAutoField(primary_key=True) # pylint: disable=invalid-name | ||
| id = models.BigAutoField(primary_key=True) # pylint: disable=invalid-name | ||
| user_id = models.IntegerField(blank=False, db_index=True) | ||
| course_id = CourseKeyField(blank=False, max_length=255) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change absolutely necessary?
UnsignedBigIntAutoFieldshouldn't be used in anything new going forward, but changing the existing field types like this sounds like it would introduce a lot of operational risk for people who have existing sites.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ormsbee I have tested it by generating migrations for existing mysql setup, and it is not detecting it as a database change because that column is already a bigint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A
BigAutoFieldwill generate abigintcolumn in MySQL, whileUnsignedBigIntAutoFieldwill make abigint unsigned. Even if it's not detected as a change because it's a custom field, changing the field types like that may cause unintended consequences down the line for existing installs. For instance, say someone develops a plugin and the models include a foreign key reference toStudentModule. They're developing locally with a new dev install, so their plugin migration makes abigintforeign key toStudentModule, and things seem to work. But when they try to package up their plugin and install it on a long-running install, that foreign key field type is incompatible because it's connecting to the existingStudentModule.idwhich isbigint unsigned.We also can't alter the type of the existing table in a migration because people have installs that have billions of rows in these tables, and doing this would require locking rebuilding the table and its indexes.
So I know it's ugly, but please bring back the
UnsignedBigIntAutoFieldso that this field's column type in MySQL remains exactly the same before and after this PR, whether it's a new or existing install.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
your commend make a lot of sense I am going to put it back. But I still feel it's not going to be a problem as the migration is already using
models.AutoFIeldso It is already having django default behaviour.0011_csm_id_bigint.pyis the only migration which is usinglms.djangoapps.courseware.fields.UnsignedBigIntAutoFieldhowever it's a history table and not going to conflict.However I am putting it back so we get sometime to think over this issue and resolve separately.