fix: migrations to make it postgresql compatible. by qasimgulzar · Pull Request #2281 · openedx/edx-enterprise

qasimgulzar · 2024-11-04T08:25:05Z

Summary

This pull request includes updates to several migration files across different integrated channels to handle database-specific operations for creating and dropping indexes. The changes ensure compatibility with various database engines, including SQLite, PostgreSQL, and MySQL.

Key Changes:

Database-Specific Index Handling:

Added conditional logic to handle index creation and dropping for PostgreSQL and MySQL in integrated_channels/blackboard/migrations/0013_alter_blackboardlearnerdatatransmissionaudit_index_together.py.
Implemented similar changes for integrated_channels/canvas/migrations/0028_alter_canvaslearnerdatatransmissionaudit_index_together.py [1] integrated_channels/cornerstone/migrations/0025_alter_cornerstonelearnerdatatransmissionaudit_index_together.py [2] integrated_channels/degreed/migrations/0027_alter_degreedlearnerdatatransmissionaudit_index_together.py [3] and integrated_channels/degreed2/migrations/0019_alter_degreed2learnerdatatransmissionaudit_index_together.py [4].

Refactoring for Index SQL Generation:

Refactored index creation and dropping SQL generation into functions for better readability and maintainability in integrated_channels/moodle/migrations/0023_alter_moodlelearnerdatatransmissionaudit_index_together.py.
Applied a similar approach in integrated_channels/sap_success_factors/migrations/0011_alter_sapsuccessfactorslearnerdatatransmissionaudit_index_together.py.

Table Name Specification:

Added db_table specification to the Meta class in the model to ensure the correct table name is used in integrated_channels/sap_success_factors/models.py.

These changes improve the robustness and compatibility of the migration scripts across different database backends.

openedx-webhooks · 2024-11-04T08:25:08Z

Thanks for the pull request, @qasimgulzar!

This repository is currently maintained by @openedx/2u-enterprise.

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 approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where 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:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

qasimgulzar · 2024-11-13T13:34:58Z

@ormsbee could you please approve the workflow execution?

ormsbee · 2024-11-15T16:22:47Z

@qasimgulzar: @iloveagent57 said that he could take a look at this one.

mphilbrick211 · 2024-12-10T22:33:02Z

@qasimgulzar - just flagging some failing checks here.

qasimgulzar · 2024-12-11T07:03:31Z

I will take care of it

mphilbrick211 · 2025-02-24T22:45:43Z

Hi @openedx/2u-enterprise! Would someone be able to take a look at this?

mphilbrick211 · 2025-03-19T16:25:39Z

Hi @openedx/2u-enterprise! Would someone be able to take a look at this?

Friendly ping on this @openedx/enterprise-titans @openedx/2u-enterprise

ormsbee

Some questions and a few minor (optional) nit suggestions.

ormsbee · 2025-03-20T16:07:37Z

...ls/blackboard/migrations/0013_alter_blackboardlearnerdatatransmissionaudit_index_together.py

-                ]
-            ),
-        ]
+        if 'postgresql' in db_engine:


Nit: I think this would read a bit easier if it was in an "elif" so that each set of operations is at the same indentation level, i.e.:

if 'sqlite3' in db_engine: # ... elif 'postgresql' in db_engine: # .... else: # ...

I would agree it make sense.

ormsbee · 2025-03-20T16:10:38Z

.../cornerstone/migrations/0025_alter_cornerstonelearnerdatatransmissionaudit_index_together.py

+                    ],
+                    database_operations=[
+                        migrations.RunSQL(sql="""
+                            CREATE INDEX cornerstone_cldta_85936b55_idx


Question: Why not CREATE INDEX CONCURRENTLY here?

I believe it will be fresh start of database if anyone will be choosing to go with Postgresql and there will be no pre-populated data. However If I use CONCURRENTLY there is a potential if could have error in background which is very unlikely but there is a chance. So I believe it will only add complexity with no benefits

ormsbee · 2025-03-20T16:12:19Z

...channels/degreed/migrations/0027_alter_degreedlearnerdatatransmissionaudit_index_together.py

+                ],
+                database_operations=[
+                    migrations.RunSQL(sql="""
+                        CREATE INDEX degreed_dldta_85936b55_idx


Question: Why not CREATE INDEX CONCURRENTLY here?

I believe it will be fresh start of database if anyone will be choosing to go with Postgresql and there will be no pre-populated data. However If I use CONCURRENTLY there is a potential if could have error in background which is very unlikely but there is a chance. So I believe it will only add complexity with no benefits

ormsbee · 2025-03-20T16:14:12Z

...annels/degreed2/migrations/0019_alter_degreed2learnerdatatransmissionaudit_index_together.py

+    else:
+        operations = [
+            migrations.AlterIndexTogether(
+                name='degreed2learnerdatatransmissionaudit',
+                index_together={('enterprise_customer_uuid', 'plugin_configuration_id')},
+            ),
+        ]


I'm confused by this. Why is this here? When would this ever execute for us, if it's not sqlite, postgres, or mysql?

you are right I might have overthink here I can cover sqlite with else only.

ormsbee · 2025-03-20T16:46:26Z

.../integrated_channel/migrations/0028_alter_contentmetadataitemtransmission_unique_together.py

-                        CREATE UNIQUE INDEX content_transmission_channel_code_plugin_id_content_id_unique
-                        ON integrated_channel_contentmetadataitemtransmission (integrated_channel_code, plugin_configuration_id, content_id)
-                        ALGORITHM=INPLACE LOCK=NONE
+        if 'postgresql' in db_engine:


Same nit about using elif to reduce indentation here.

ormsbee · 2025-03-20T16:49:06Z

...d_channels/moodle/migrations/0023_alter_moodlelearnerdatatransmissionaudit_index_together.py

+            ON moodle_moodlelearnerdatatransmissionaudit (enterprise_customer_uuid, plugin_configuration_id)
+        """
+    else:
+        raise ValueError("Unsupported database engine")


Please add a comment about why it's okay to not account for sqlite3 here.

ormsbee · 2025-03-20T16:49:41Z

..._0011_alter_sapsuccessfactorslearnerdatatransmissionaudit_enterprise_course_enrollment_id.py

+            options={
+                'db_table': 'sap_success_factors_sapsuccessfactorslearnerdatatransmission3ce5',
+            }


Question: Why was this change necessary?

If don't do this change the table name generated in postgresql doesn't match with the table name it generates in mysql. I believe it is because of the make table name length constraints.
The reason of pinning it is as we are using the table name while creating index in subsequent migration.
0011_alter_sapsuccessfactorslearnerdatatransmissionaudit_index_together.py

ormsbee · 2025-03-20T16:50:50Z

...actors/migrations/0011_alter_sapsuccessfactorslearnerdatatransmissionaudit_index_together.py

+        return """
+                        CREATE INDEX sapsf_sldta_85936b55_idx
+                        ON sap_success_factors_sapsuccessfactorslearnerdatatransmission3ce5 (enterprise_customer_uuid, plugin_configuration_id)
+                    """


NIt: I think this is okay either way, but if it's only being used once, I think you might as well inline it like you did elsewhere in this PR.

ormsbee · 2025-03-20T16:51:10Z

...rated_channels/xapi/migrations/0011_alter_xapilearnerdatatransmissionaudit_index_together.py

-                        ON xapi_xapilearnerdatatransmissionaudit (enterprise_customer_uuid, plugin_configuration_id)
-                        ALGORITHM=INPLACE LOCK=NONE
-                    """, reverse_sql="""
+                    migrations.RunSQL(sql=generate_index_sql(db_engine), reverse_sql="""


Nit: I think this is okay either way, but if it's only being used once, I think you might as well inline it like you did elsewhere in this PR.

qasimgulzar · 2025-03-28T09:55:39Z

@ormsbee I had accommodated your feedback please give it a look again. Also I have responded to few of your questions.

ormsbee

I'll merge when tests pass.

ormsbee · 2025-04-07T20:22:29Z

@openedx/2u-enterprise: I'm planning to merge this tomorrow. Please let me know if anyone wants to look it over before I do so. Thank you.

qasimgulzar · 2025-04-07T20:27:40Z

@ormsbee could you please approve the tests again?

qasimgulzar · 2025-04-08T08:56:20Z

@ormsbee could you please approve the workflows now?

iloveagent57 · 2025-04-08T14:28:29Z

@qasimgulzar @ormsbee approved by enterprise, workflows are running now.

iloveagent57 · 2025-04-08T14:43:33Z

@qasimgulzar could you squash the commits, add a CHANGELOG entry, and bump the version, please, and then this will be ready to merge. Thanks!

qasimgulzar · 2025-04-09T05:21:07Z

@iloveagent57 sure I will do it today

qasimgulzar · 2025-04-09T05:32:15Z

@iloveagent57 I just pushed these changes.

Update CHANGELOG.rst
Bump Version [5.12.4]
Squashed the commits

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 4, 2024

qasimgulzar mentioned this pull request Nov 4, 2024

fix: migrations to make postgresql compatible. openedx/openedx-platform#35762

Merged

5 tasks

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 8, 2024

qasimgulzar mentioned this pull request Nov 18, 2024

Add PostgreSQL support openedx/openedx-core#255

Open

qasimgulzar force-pushed the qasim/migrations/postgresql branch from 8c0641b to 3516afa Compare November 22, 2024 11:55

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 10, 2024

qasimgulzar force-pushed the qasim/migrations/postgresql branch from 88112fd to 3e30cce Compare December 12, 2024 11:21

mphilbrick211 requested a review from a team February 24, 2025 22:45

qasimgulzar force-pushed the qasim/migrations/postgresql branch from 3e30cce to 7496e15 Compare February 25, 2025 07:22

ormsbee requested changes Mar 20, 2025

View reviewed changes

qasimgulzar requested a review from ormsbee March 23, 2025 08:08

ormsbee approved these changes Apr 7, 2025

View reviewed changes

qasimgulzar force-pushed the qasim/migrations/postgresql branch from 7d83fc3 to 3531071 Compare April 7, 2025 19:40

iloveagent57 approved these changes Apr 8, 2025

View reviewed changes

fix: migrations to make it postgresql compatible.

71abea6

qasimgulzar force-pushed the qasim/migrations/postgresql branch from d9bb855 to 71abea6 Compare April 9, 2025 05:29

ormsbee merged commit 2a6a89a into openedx:master Apr 9, 2025
9 of 11 checks passed

github-project-automation bot moved this from Ready for Review to Done in Contributions Apr 9, 2025

qasimgulzar deleted the qasim/migrations/postgresql branch May 5, 2025 20:19

Conversation

qasimgulzar commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Changes:

Database-Specific Index Handling:

Refactoring for Index SQL Generation:

Table Name Specification:

Uh oh!

openedx-webhooks commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qasimgulzar commented Nov 13, 2024

Uh oh!

ormsbee commented Nov 15, 2024

Uh oh!

mphilbrick211 commented Dec 10, 2024

Uh oh!

qasimgulzar commented Dec 11, 2024

Uh oh!

mphilbrick211 commented Feb 24, 2025

Uh oh!

mphilbrick211 commented Mar 19, 2025

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qasimgulzar commented Mar 28, 2025

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

ormsbee commented Apr 7, 2025

Uh oh!

qasimgulzar commented Apr 7, 2025

Uh oh!

qasimgulzar commented Apr 8, 2025

Uh oh!

iloveagent57 commented Apr 8, 2025

Uh oh!

iloveagent57 commented Apr 8, 2025

Uh oh!

qasimgulzar commented Apr 9, 2025

Uh oh!

qasimgulzar commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

qasimgulzar commented Nov 4, 2024 •

edited

Loading

openedx-webhooks commented Nov 4, 2024 •

edited

Loading

qasimgulzar commented Apr 9, 2025 •

edited

Loading