fix(Segments): Restore segment list performance on large installations - #8587
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe v2 filter in Estimated code review effort: 2 (Simple) | ~8 minutes Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk is established by the supplied review context; the change is ready to merge subject to normal checks. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Docker builds report
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 3281cb46-4638-4056-97c2-506c18c4df75
📒 Files selected for processing (2)
api/segments/services.pyapi/tests/unit/segments/test_unit_segments_services.py
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8587 +/- ##
==========================================
- Coverage 98.81% 98.80% -0.01%
==========================================
Files 1631 1631
Lines 67012 67013 +1
==========================================
- Hits 66215 66214 -1
- Misses 797 799 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20766 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #20766 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #20766 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20766 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
Skipped testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss ✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20765 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20765 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20765 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #20765 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20764 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20764 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #20764 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20764 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
`get_all_live_or_scheduled_overrides` matched a feature state's version against `EnvironmentFeatureVersion.objects.get_live_or_scheduled()`, an uncorrelated subquery over every published version in the installation. On a reported project, the `has_overrides` annotation on the segment list took 59 seconds to return 156 rows, almost all of it spent in a merge anti-join reading ~90k and ~147k version rows, 313 times over. Correlate the superseded-version check with the feature state's own feature and environment instead, which the existing `efv_env_feature_pub_created` index already covers. The same query then runs in 3.6ms, and the planner drops the merge joins that scanning drove it to, reading `features_featuresegment` by segment rather than by primary key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2f444dd to
07463d5
Compare
emyller
left a comment
There was a problem hiding this comment.
To be fair, I still don't grasp why the filter needs to be not superseded instead of superseding, but the logic here looks sound.
for more information, see https://pre-commit.ci
Drop the module-level constant and the helper's return value, and assert the exact number of version rows the plan reads rather than a fraction of the table's size. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 18c1b18a-4fb3-4780-9abf-7353a7ba6a01
📒 Files selected for processing (2)
api/segments/services.pyapi/tests/unit/segments/test_unit_segments_services.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Since #8548,
get_all_live_or_scheduled_overridescompared a feature state's version againstEnvironmentFeatureVersion.objects.get_live_or_scheduled()— an uncorrelated subquery over every published version in the installation. Correlating the check to the feature state's own feature and environment lets the existingefv_env_feature_pub_createdindex serve it, so there is no migration.How did you test this code?
EXPLAIN (ANALYZE)on a production read replica, listing segments for a project with 156 of them.Before — 59,069 ms.
SubPlan 1is the version check, reading ~90k and ~147k version rows, 313 times over. Note alsofeatures_featuresegmentscanned by primary key, andfeatures_featurestategathered across the whole table:After — 3.6 ms.
SubPlan 1is now an index seek and is never reached; the other two joins flip to nested loops on the right indexes:The existing
get_all_live_or_scheduled_overridestests cover the semantics and pass unchanged. The new test asserts how many version rows the plan reads: 1 with this change, 4,002 without.