[PM-21044] - optimize security task ReadByUserIdStatus #5779
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5779 +/- ##
==========================================
- Coverage 51.01% 50.95% -0.06%
==========================================
Files 1665 1665
Lines 75074 75117 +43
Branches 6764 6772 +8
==========================================
- Hits 38299 38276 -23
- Misses 35252 35322 +70
+ Partials 1523 1519 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job, no security vulnerabilities found in this Pull Request |
| WHERE | ||
| @Status IS NULL | ||
| OR ST.Status = @Status | ||
| ) | ||
| SELECT |
There was a problem hiding this comment.
The original had a group by which suggested there could be duplicates, do we need a distinct here?
There was a problem hiding this comment.
@rkac-bw The UNION ALL (no dedupe) and a NOT EXISTS semi‑join will exclude duplicates.
There was a problem hiding this comment.
Interesting, the procedure uses UNION ALL between the UserCollectionAccess and filtered GroupCollectionAccess CTEs, which could potentially introduce duplicates. However, the NOT EXISTS subquery in the GroupCollectionAccess CTE and the final left join should make it unique, looks good
| FROM UserCollectionAccess AS UA | ||
| WHERE UA.CipherId = GC.CipherId | ||
| ) | ||
| ) |
There was a problem hiding this comment.
After the AccessibleCiphers CTE, you need a comma before defining the SecurityTasks CTE. Without this comma, the query fails?
There was a problem hiding this comment.
@rkac-bw Thank you for pointing this out!
There was a problem hiding this comment.
Changes to the Sproc look good! Just need to make a small change to the migration script.
Added Four targeted non‑clustered indexes. Eliminated scans on CollectionGroup, SecurityTask, etc., and covered every lookup.
I'm not seeing any new indexes being created as mentioned in the PR description?
@shane-melton 🤦 It's been so long since I put this together I completely forgot to add them. They're now added. |
src/Sql/dbo/Vault/Stored Procedures/SecurityTask/SecurityTask_ReadByUserIdStatus.sql
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Sproc change looks good to me. I'm less familiar with the impacts of the new indexes being created.
@rkac-bw would you mind taking a look at the new nonclustered indexes being added?
Indeed they could be overkill. I'm not married to them if you guys prefer to leave them out. ;) |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-21044
📔 Objective
The original query What we had was a single flat query with numerous JOINs and a GROUP BY,. This fo 8000 rced the optimizer to consider large intermediate rowsets and perform hidden de‑duping and scans.
PLEASE NOTE - I'm not familiar with db migrations so they're currently absent
After taking @shane-melton's initial plan (see ticket) to use focused CTE's I added further refinements:
WHERE ST.Status = COALESCE(@Status, ST.Status)→ non‑sargable, forced scans/filtersWHERE @Status IS NULL OR ST.Status = @Status→ fully sargable, pushes directly into an Index Seek on (Status, OrganizationId, CreationDate DESC) and eliminates scan + downstream SORT.UNION ALL(no dedupe) and a simpleNOT EXISTSsemi‑join to exclude duplicates before the append—no Sort or Stream Aggregate neededOR EXISTS (SELECT 1 FROM AccessibleCiphers AC WHERE AC.CipherId = ST.CipherId)(correlated subquery per row)LEFT JOIN AccessibleCiphers AC ON ST.CipherId = AC.CipherIdandWHERE ST.CipherId IS NULL OR AC.CipherId IS NOT NULL. This flattens the plan into one nested‑loops join over the small CTE, cutting I/O and simplifying the optimizer’s job.ORDER BY ST.CreationDate DESCalways triggered a SORT operatorCreationDate DESCin the third key of our covering IX_SecurityTask_Status_OrgId_CreationDateDesc index, the engine can return rows in descending order directly—no separate SORT required.Added Four targeted non‑clustered indexes. Eliminated scans on CollectionGroup, SecurityTask, etc., and covered every lookup.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes