fix: migrations to make postgresql compatible. #35762
Conversation
|
Thanks for the pull request, @qasimgulzar! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
the
open-source-contribution
qasimgulzar
force-pushed
the
qasim/postgres
branch
3 times, most recently
from
b8bc93e to
eae8dd3
Compare
|
@ormsbee I am waiting on it's subsequent PRs to be merged before moving ahead for further implementations. |
Update.-.1.1.-.min.mov |
|
I will take care of the broken tests |
qasimgulzar
force-pushed
the
qasim/postgres
branch
3 times, most recently
from
47d7ce7 to
e47f7fd
Compare
|
@ormsbee can we manage review for the subsequent PRs? |
qasimgulzar
force-pushed
the
qasim/postgres
branch
2 times, most recently
from
005c29d to
6d0f777
Compare
qasimgulzar
force-pushed
the
qasim/postgres
branch
from
6d0f777 to
2e53240
Compare
|
Two PRs are left to be reviewed and merged. |
|
@ormsbee friendly ping on this! |
@AntonOfTheWoods once this PR will be merged we are expecting edx-platform repo to support postgresql. I will be focusing on other part of it once it's merged. Meanwhile let me know if I can be helpful for you. |
@qasimgulzar thanks, I think I just need the remaining repos' support now. I just added the |
|
@AntonOfTheWoods i believe the PR for openedx-learning is already merged. Around that issue. Oh may be you are right the case sensitive thing. Let me test that |
|
For openedx-learning, case sensitive and case insensitive fields are usually generated from this helper module, which would probably be a good place to put in postgres-specific logic: https://github.com/openedx/openedx-learning/blob/main/openedx_learning/lib/fields.py You can see how it's done for MySQL and sqlite today: |
|
@ormsbee This PR is ready for another iteration, probably we can handle case insensitive index in separate PR as that is not a blocker for this PR to be merged. Please let me know if you need any other updates. |
Flagging this, @ormsbee @AntonOfTheWoods |
|
@ormsbee it's just a nudge |
|
@mphilbrick211 , @ormsbee seems very busy. Maybe there is another maintainer who could take a look? The PR seems to have been opened over a year ago now and many other PG-related PRs seem to have already been merged. |
|
Sorry folks, I'll do another pass by end of day Tuesday. Thank you. |
|
|
||
| def adapt_course_locator(course_locator): | ||
|
|
||
| return QuotedString(course_locator._to_string()) # lint-amnesty, pylint: disable=protected-access |
There was a problem hiding this comment.
Sorry, I mean why course_locator._to_string()? But if you're deleting it anyway, it's a moot point.
| ] | ||
|
|
||
| operations = [ | ||
| migrations.CreateModel( |
There was a problem hiding this comment.
If openedx/openedx-learning#422 merges, would this be re-done in that style?
There was a problem hiding this comment.
I can certainly take care of that if desired.
There was a problem hiding this comment.
@AntonOfTheWoods could you look into this point if you have sometime.
There was a problem hiding this comment.
@qasimgulzar I certainly will but I'd very much like someone more familiar with the code to comment on that PR. One of the key points is that your current code is, unless I'm mistaken, pg14+ compatible.
Officially that draft PR of mine doesn't do everything that the mysql version does - notably, it simply throws away any indexes on case-insensitive columns. Postgresql Has CIText for that but it's been "deprecated" for many years and the field type has already been removed from Django core. As such, using that would require quite a bit of extra thought and work.
Adding index-supported case-insensitive columns for postgres would require significant extra work and testing for <pg18, and as I mentioned in the PR, I couldn't see anywhere it could make a difference. I'm an openedx n00b though. If we forget about supporting anything less than pg18, then I think we can add the indexes back. I looked at that almost 2 months ago now, so there may have been an extra reason for indexes not working exactly the same with pg18+.
The overwhelming majority of the changes on that PR are just making the code DRYer (using a pre-existing function that could have been used previously anyway), so there isn't actually that much to review - the main point is the "silent" removing of the index.
There was a problem hiding this comment.
@ormsbee after giving a look to PR and above comment, We don't really need to re-do any thing in this context.
lms/djangoapps/grades/models.py
Outdated
|
|
||
| # 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 |
There was a problem hiding this comment.
Okay, maybe I'm just being dense here, but won't this still be a problem if I have an existing site and want to later add a foreign key that references this model? Because my existing site will have this primary key in the database as type "unsigned bigint". But the code here says it's actually an "bigint" now. So then when I make a new model that has a foreign key to PersistentSubsectionGrade, won't the migration try to generate that foreign key as type "bigint"? Which will work fine on new installs, but blow up if I have an older site?
There was a problem hiding this comment.
I know you went over this before in an earlier thread, but I think there's something I'm just not understanding properly.
Thank you for your patience.
There was a problem hiding this comment.
@ormsbee That seems correct to me, but I haven't tested this kind of thing in years.
There was a problem hiding this comment.
@ormsbee I have tested it with mysql and it is not treating it as type change, I also have updated the underlaying migrations to use this type so that it doesn't cause any discrepancy for new installs. for existing users these migrations also not going to trigger even so it seems safe.
If you have any particular scenario please let me know.
|
@qasimgulzar , just a gentle nudge... |
|
I will look into it over the weekend. |
|
@ormsbee it's a nudge |
AntonOfTheWoods
left a comment
AntonOfTheWoods
left a comment
There was a problem hiding this comment.
Looks ok now. There is now only one not-strictly-related change which seems like an obvious bugfix.
@qasimgulzar as a general rule, the bigger the PR, the bigger the risk and the longer it's going to take to get accepted. So make PRs as small and independent as possible so they can get merged quickly and not need 1+ years...
| operations.append( | ||
| migrations.AddField( | ||
| model_name='courseoverview', | ||
| name='facebook_url', | ||
| field=models.TextField(null=True), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
If this is an unrelated bug (even if it's necessary for pg to work), I would put it in a separate, neutral, bugfix PR that should get merged before the new stuff.
From the look of the code, you might argue that there is already pg-specific code and at some point in the past pg probably was supported and that support was never really properly dropped (or there wouldn't be any pg-specific code left). So this whole PR is actually just a bugfix, making pg work properly again. But I'm not sure the maintainers see it this way :-).
There was a problem hiding this comment.
I am going to take care of it in other PR, as it is not tightly coupled with this change.
8 participants
Summary
This pull request includes several important changes to improve database migrations and remove the
UnsignedBigIntAutoFieldcustom field. The key changes involve modifying SQL commands to support different database engines and ensuring atomic transactions during migrations. The most important changes are summarized below:Database Migration Improvements:
common/djangoapps/split_modulestore_django/migrations/0001_initial.py: Added functions to generate SQL commands for MySQL and PostgreSQL to ensure thecourse_idcolumn is case-sensitive. [1] [2]lms/djangoapps/commerce/migrations/0001_data__add_ecommerce_service_user.py: Wrapped user creation and deletion operations in atomic transactions to ensure database consistency.Field Type Changes:
lms/djangoapps/courseware/models.py,lms/djangoapps/courseware/migrations/0001_initial.py,lms/djangoapps/coursewarehistoryextended/models.py,lms/djangoapps/coursewarehistoryextended/migrations/0001_initial.py,lms/djangoapps/grades/models.py,lms/djangoapps/grades/migrations/0001_initial.py: ReplacedUnsignedBigIntAutoFieldwithmodels.BigAutoFieldto standardize the primary key field types. [1] [2] [3] [4] [5] [6]Code Cleanup:
lms/djangoapps/courseware/fields.py: Removed theUnsignedBigIntAutoFieldcustom field as it is no longer needed.lms/djangoapps/grades/migrations/0015_historicalpersistentsubsectiongradeoverride.py: Added missing fieldgradeto theHistoricalPersistentSubsectionGradeOverridemodel.These changes ensure better compatibility with different database engines and improve the consistency and maintainability of the codebase.
Subsequent Pull Requests