Repository navigation
Optimized GetHostScriptExecutionResults MySQL query for for large numbers of script results. - #32595
Conversation
…bers of script results.
…-list-script-results # Conflicts: # server/datastore/mysql/schema.sql
…lts to improve query performance.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32595 +/- ##
==========================================
- Coverage 64.01% 64.01% -0.01%
==========================================
Files 2000 2001 +1
Lines 198159 198188 +29
Branches 6462 6462
==========================================
+ Hits 126859 126877 +18
- Misses 61310 61320 +10
- Partials 9990 9991 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…readability and efficiency by using window functions and simplifying logic.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds a migration and schema changes to replace an index on host_script_results and updates the SQL in GetHostScriptDetails to compute the latest per-script activity using CTEs and window functions. A changelog entry documents the optimization. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client/UI
participant API as Server API
participant DS as Datastore (scripts.go)
participant DB as MySQL
UI->>API: GET /hosts/{id}/scripts
API->>DS: GetHostScriptDetails(hostID, team/global scope)
DS->>DB: SQL with CTEs + window functions<br/>uses idx_host_script_canceled_created_at
DB-->>DS: Latest per-script rows (upcoming or completed)
DS-->>API: Script list with latest activity fields
API-->>UI: 200 OK response
note over DB,DS: New composite index (host_id, script_id, canceled, created_at DESC)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
server/datastore/mysql/migrations/tables/20250904115553_OptimizeHostScriptResultsIndex.go (1)
30-35: Create-before-drop is the right order; consider large-table lock impact (optional).If you expect very large host_script_results tables, using ALTER TABLE ... ADD INDEX with ALGORITHM/LOCK options can further reduce lock time in some MySQL setups. Not blocking.
server/datastore/mysql/scripts.go (4)
973-1022: CTE/window approach is sound; consider naming columns explicitly.SELECT * in both ranked subqueries can increase materialization work. Listing explicit columns keeps the shape tight and resilient to future additions. Optional.
Apply this diff skeleton (adjust types as needed):
- SELECT - id, - host_id, - script_id, - execution_id, - created_at, - exit_code, - 'completed' as source, + SELECT + id, + host_id, + script_id, + execution_id, + created_at, + exit_code, + 'completed' AS source, ... - SELECT - NULL as id, - ua.host_id, - sua.script_id, - ua.execution_id, - ua.created_at, - NULL as exit_code, - 'upcoming' as source, + SELECT + NULL AS id, + ua.host_id, + sua.script_id, + ua.execution_id, + ua.created_at, + NULL AS exit_code, + 'upcoming' AS source,
1033-1047: Semantics: “prefer upcoming over completed” even if older — confirm intent.The CASE puts upcoming ahead of completed regardless of timestamps. If the goal is “show the most recent activity overall,” consider preferring upcoming only when it’s newer.
Alternative ORDER if you want “newest overall, but upcoming wins only on tie”:
-ORDER BY - CASE WHEN source = 'upcoming' THEN 1 ELSE 2 END, - created_at DESC, - id DESC +ORDER BY + created_at DESC, + id DESC, + CASE WHEN source = 'upcoming' THEN 1 ELSE 2 END
1048-1052: Redundant host filter; simplify (optional).latest already contains rows filtered by host_id in the CTE. The WHERE (latest.host_id IS NULL OR latest.host_id = ?) is unnecessary and can be dropped to simplify args.
Apply this diff:
-WHERE - (latest.host_id IS NULL OR latest.host_id = ?) - AND s.global_or_team_id = ? +WHERE + s.global_or_team_id = ?And update args accordingly (see next comment).
--- `1054-1060`: **Adjust args if previous simplification is applied.** If you drop the extra host filter, remove one hostID from args. Apply this diff: ```diff -args := []any{hostID, hostID, hostID, globalOrTeamID} +args := []any{hostID, hostID, globalOrTeamID} ... -if len(extension) > 0 { - args = append(args, extension) +if len(extension) > 0 { + args = append(args, extension)(No functional change apart from arg count.)
</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d7fa824e97f377abaae0d1f21bd9a0af847bf31b and 471b0fdfbc874e067867df4b74b49d5c68074133. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `changes/32296-optimize-list-script-results` (1 hunks) * `server/datastore/mysql/migrations/tables/20250904115553_OptimizeHostScriptResultsIndex.go` (1 hunks) * `server/datastore/mysql/schema.sql` (2 hunks) * `server/datastore/mysql/scripts.go` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**/*.go</summary> **⚙️ CodeRabbit configuration file** > When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping. Files: - `server/datastore/mysql/migrations/tables/20250904115553_OptimizeHostScriptResultsIndex.go` - `server/datastore/mysql/scripts.go` </details> </details><details> <summary>🧠 Learnings (2)</summary> <details> <summary>📚 Learning: 2025-07-08T16:13:39.114Z</summary>Learnt from: getvictor
PR: #30589
File: server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go:53-55
Timestamp: 2025-07-08T16:13:39.114Z
Learning: In the Fleet codebase, Down migration functions are intentionally left empty/no-op. The team does not implement rollback functionality for database migrations, so empty Down_* functions in migration files are correct and should not be flagged as issues.**Applied to files:** - `server/datastore/mysql/migrations/tables/20250904115553_OptimizeHostScriptResultsIndex.go` </details> <details> <summary>📚 Learning: 2025-08-01T15:08:16.858Z</summary>Learnt from: sgress454
PR: #31508
File: server/datastore/mysql/schema.sql:102-116
Timestamp: 2025-08-01T15:08:16.858Z
Learning: The schema.sql file in server/datastore/mysql/ is auto-generated from migrations for use with tests, so it cannot be manually edited. Any changes must be made through migrations.**Applied to files:** - `server/datastore/mysql/schema.sql` </details> </details> </details> <details> <summary>🔇 Additional comments (5)</summary><blockquote> <details> <summary>server/datastore/mysql/migrations/tables/20250904115553_OptimizeHostScriptResultsIndex.go (2)</summary><blockquote> `22-27`: **Good composite index; matches query access pattern.** (host_id, script_id, canceled, created_at DESC) aligns with the new “latest-per-script-per-host” scans and canceled=0 filtering. --- `40-43`: **No-op Down is consistent with Fleet’s migration policy.** Matches the team’s standard (no rollbacks). </blockquote></details> <details> <summary>server/datastore/mysql/schema.sql (3)</summary><blockquote> `830-830`: **Index matches migration and ordering.** New idx_host_script_canceled_created_at is present with DESC on created_at; consistent with the migration. --- `1498-1498`: **AUTO_INCREMENT bump looks correct.** AUTO_INCREMENT=419 matches the added migration row. --- `1500-1501`: **New migration row recorded.** Entry for version 20250904115553 is present; schema.sql stays in sync with migrations. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
dantecatalfamo
left a comment
There was a problem hiding this comment.
Cool use of window function
…bers of script results. (#32595) Fixes #32295 The issue was identified/fixed using this performance test: https://github.com/proxy/gist.github.com/getvictor/b289b7b14981fb7bf77e57c80af117d1 With the fix: - 100 records: 2.6ms (similar) - 1,000 records: ~7ms (32x faster) - 5,000 records: ~10ms (530x faster) - 10,000 records: 13ms (1,430x faster) - 20,000 records: ~25ms (2,960x faster) - 40,000 records: 50ms (6,000x faster) # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Script details now surface the most relevant “latest” status per script, prioritizing upcoming executions when present. - Performance Improvements - Significantly faster loading of host script results and script details, especially at large scale. - Improved responsiveness when filtering/sorting script results. - Documentation - Added changelog entry describing the optimization to script results handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit 73b629e)
Fixes #32295
The issue was identified/fixed using this performance test: https://github.com/proxy/gist.github.com/getvictor/b289b7b14981fb7bf77e57c80af117d1
With the fix:
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit