-
Notifications
You must be signed in to change notification settings - Fork 84
[ENG-2046] #7035
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
[ENG-2046] #7035
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile OverviewGreptile SummaryAdds database index on
Confidence Score: 5/5
Important Files ChangedFile Analysis
|
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.
2 files reviewed, no comments
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7035 +/- ##
=======================================
Coverage 87.00% 87.01%
=======================================
Files 528 528
Lines 34678 34682 +4
Branches 4010 4010
=======================================
+ Hits 30173 30177 +4
Misses 3629 3629
Partials 876 876 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
erosselli
left a comment
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.
Thanks for addressing this so quickly! I left two comments but the only one I care about is the first one about adding the index to the model :) approving with that in mind
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.
I think we can also add the index declaratively on the model with __table_args__ , that way anyone who looks at the model know what indexes are available without needing to look at the DB or dig through migrations
| sa.text("SELECT COUNT(*) FROM providedidentity") | ||
| ).scalar() | ||
|
|
||
| if providedidentity_count < 1000000: |
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.
iirc this number is kind of arbitrary... but also I don't have a more specific one so 🤷♀️
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.
I mostly picked it because it was already being used in other places. I looked at the post_upgrade_index_creation.py file and tried to use the same pattern as migrations for tables in that list. Its used in at least 5 other migrations.
I can add a variable to that file so we have it documented on one spot though for future cases.
…ethod-on-a-privacy-request-causes-a-full-table-scan
…ethod-on-a-privacy-request-causes-a-full-table-scan
…ethod-on-a-privacy-request-causes-a-full-table-scan
Co-authored-by: Jade Wibbels <jade@ethyca.com>
Ticket ENG-2046
Description Of Changes
🎯 We currently don’t have an index on
providedidentity.privacy_request_id, which causes us to run a full table scan any time we call a privacy request’s get_persisted_identity method.More context on this slack thread
AC
Added a database index on the providedidentity.privacy_request_id foreign key column
For large tables (≥ 1M rows), index is automatically created using
CREATE INDEX CONCURRENTLYon application startup viapost_upgrade_index_creation.pyDROP INDEX IF EXISTSto handle cases where index may not existTABLE_OBJECT_MAPinpost_upgrade_index_creation.pywith comment noting it can be removed once all deployments have upgradedCode Changes
/src/fides/api/alembic/migrations/versions/xx_2025_11_25_1854_3ff6449c099e_add_index_on_providedidentity_privacy_.py/src/fides/api/migrations/post_upgrade_index_creation.pySteps to Confirm
You should get:
indexname | indexdef -----------+---------- (0 rows)Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works