Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Make app AutoField configurable on first deployment #465

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
Loading
from

Conversation

browniebroke
Copy link
Contributor

@browniebroke browniebroke commented Mar 5, 2025

While not fully addressing the problem reported in #307, it switches new installations to use a BigAutoField for primary key. Existing installations won't see any changes unless they replay the migrations and to recreate the result table.

@browniebroke browniebroke force-pushed the fix/results-auto-field-setting branch from 1d87f48 to 0d75c01 Compare March 5, 2025 00:07
@browniebroke browniebroke force-pushed the fix/results-auto-field-setting branch from 0d75c01 to 94380cb Compare March 5, 2025 00:08
@auvipy auvipy self-requested a review March 5, 2025 04:54
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.

we should also think about what if existing projects want to migrate to this

@browniebroke browniebroke marked this pull request as ready for review March 5, 2025 08:04
@browniebroke
Copy link
Contributor Author

we should also think about what if existing projects want to migrate to this

Thta's true, but that is more complicated and I don't think I want to tackle migrating existing installations in this pull request.

Such migration might not even be feasible in a database agnostic way, as writing a safe migration might need to use database specific features. The project is tested against Postgres only, but it doesn't seem to use any PG only features? Is it supposed to work on other databases backend supported by Django?

@@ -0,0 +1,19 @@
Configuration
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another setting DJANGO_CELERY_RESULTS_TASK_ID_MAX_LENGTH which should probably be added here but I'd rather do this separately if you don't mind.

@auvipy
Copy link
Member

auvipy commented Mar 12, 2025

what is your take on this PR? although it is not 100% design wise #314

@browniebroke
Copy link
Contributor Author

what is your take on this PR? although it is not 100% design wise #314

That could also be a solution to the problem described in #307, although I would argue that's solving an othogonal problem. It's a bit heavy handed for thoses who only want to customise the primary key, and new users usually won't know about the primary key overflow until it's too late.

I'm seeing a fundamental missing piece betwen both PRs: formalising django-celery-results app settings better. What do you think of the implementation in this PR? I took this pattern from this post, which is making a great case for it.

If you have another pattern you prefer, I'm happy to use another example as a blueprint. I can contribute this piece separately first.

@browniebroke
Copy link
Contributor Author

we should also think about what if existing projects want to migrate to this

There was an excellent talk at DjangoCon Europe about how to do this kind of migration safely at scale, and the TL;DR is:

  1. Add new shadow column
  2. Backfill the values
  3. Swap the columns in a transaction

The 2nd step is particularly tedious and manual. It can take a long time and has some operational implications (creates dead tuples in PostgreSQL, themselves causing spikes in VACUUM)...

IMO django-celery-results should document a recipe but this should be left out of the package. Perhaps we could link to the talk when it's out?

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.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.