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

Work around Oracle migration instability #283

Merged

Conversation

intgr
Copy link
Contributor

@intgr intgr commented Feb 1, 2022

Fixes #222 (as noted in the last comment, the issue still occurs)

Django's auto-generated index names are not stable across database engines: https://code.djangoproject.com/ticket/33483

Added explicit index names to models.py to work around that, until a fix is implemented in Django itself.

Fixes celery#222.

Django's auto-generated index names are not stable across database engines: https://code.djangoproject.com/ticket/33483

Added explicit index names to `models.py` to work around that, until a fix is implemented in Django itself.
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't the changes affect any schema migrations changes?

@intgr
Copy link
Contributor Author

intgr commented Feb 2, 2022

No, I used the same index names that Django was automatically generating on non-Oracle databases. Thus no migrations are generated.

@auvipy
Copy link
Member

auvipy commented Feb 2, 2022

@felixxm can you please tell that we are not doing wrong? as you are an django ORM/oracle back end expert O:)

Copy link
Contributor

@AllexVeldman AllexVeldman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I only verified it behaves as expected on Postgres.

@intgr
Copy link
Contributor Author

intgr commented Feb 3, 2022

until a fix is implemented in Django itself.

Unfortunately the Django issue has been closed as "wontfix". I think it's fixable, but wouldn't be pretty and I don't know that I can find the time to PoC it.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also Felix suggested you some approach to follow in django 4.0 for your specific project?

@@ -95,12 +95,18 @@ class Meta:
verbose_name = _('task result')
verbose_name_plural = _('task results')

# Explicit names to solve https://code.djangoproject.com/ticket/33483
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the ticket is won't fix, I am reluctant to include this right now. Instead, can't we define index name in the model meta to generate nicer/shorter names?

Copy link
Contributor Author

@intgr intgr Feb 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand you correctly. I am defining index names in model meta.

I can change the names to be prettier, but then it would mean a migration that drops and re-creates all indexes, not just Oracle users (there's no RenameIndex migration in Django). For users who have lots of data, that can be disruptive. But if you think it's worth it, I can do it.

Copy link
Contributor Author

@intgr intgr Feb 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually a 3rd option is to use db_index=True in field definitions, instead of defining Meta.indexes. Indexes created that way are database-engine-agnostic. But this would require a migration recreating indexes as well.

image

Copy link
Contributor

@AllexVeldman AllexVeldman Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These fields used to have db_index=True and were changed to the Meta, causing all sorts of index issues:
#161 (comment)

I agree the index names are ugly but they do match the name originally generated by Django preventing any additional index migrations, which has my preference.

@intgr
Copy link
Contributor Author

intgr commented Feb 3, 2022

Also Felix suggested you some approach to follow in django 4.0 for your specific project?

Subclassing DatabaseWrapper to override max_name_length() is an option that would solve my use case, true. But I feel like I would be trading one set of "not officially suported by Django" drawbacks for another "not officially suported by Django" solution with possibly yet unknown drawbacks. It would also require significant work on my application to migrate existing table names.

If it were a blessed solution by Django developers that all Oracle users should go this route to increase chances of compatibility, I would be willing to do it, but as it stands, I'm hesitant.

I have to wonder, am I the only person trying to use django-celery-results on Oracle, or just the first one to notice and bother with this issue. It's becoming increasingly clear that the long-long term solution is to simply migrate from Oracle to PostgreSQL. But it's not happening soon.

@auvipy
Copy link
Member

auvipy commented Feb 3, 2022

I do agree to some degree, allow me some time to properly recheck and comment.

@auvipy auvipy merged commit d36a868 into celery:master Feb 9, 2022
@intgr
Copy link
Contributor Author

intgr commented Feb 11, 2022

Thank you! Any plans for a release soon? :)

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.

makemigrations in project tries to drop and re-create indexes
3 participants