-
Notifications
You must be signed in to change notification settings - Fork 435
feat: Add GIN index on Identity.identifier
#5369
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: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Docker builds report
|
Uffizzi Ephemeral Environment
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5369 +/- ##
=======================================
Coverage 97.61% 97.62%
=======================================
Files 1237 1239 +2
Lines 42971 42990 +19
=======================================
+ Hits 41948 41967 +19
Misses 1023 1023 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
indexes = [ | ||
UnrestrictedGinIndex( | ||
OpClass(Upper("identifier"), name="gin_trgm_ops"), | ||
name="environments_identity_identifier_gin_idx", |
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.
Nit: Can we shorten the index name to just identifier_gin_idx
to avoid having to subclass? It's not immediately clear what UnrestrictedGinIndex
means just from the class name. If we do keep that class, could we add a docstring to explain its purpose?
import core.db | ||
|
||
|
||
_create_index_sql = "CREATE INDEX CONCURRENTLY IF NOT EXISTS environments_identity_identifier_gin_idx ON environments_identity USING GIN (UPPER(identifier) gin_trgm_ops);" |
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 there a reason why we are not using https://docs.djangoproject.com/en/4.2/ref/contrib/postgres/operations/#django.contrib.postgres.operations.AddIndexConcurrently?
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
This PR improves
/api/v1/environments/{environment_api_key}/identities/
when searching through large quantities of identities.How did you test this code?
Made sure the migration works as expected locally.
The index has been tested in staging and is working in prod for one of the enterprise customers.