Optimize ReparseWindowsHostCertificates migration to fix DB overload during upgrade - #50262
Draft
sharon-fdm wants to merge 2 commits into
Draft
Optimize ReparseWindowsHostCertificates migration to fix DB overload during upgrade#50262sharon-fdm wants to merge 2 commits into
sharon-fdm wants to merge 2 commits into
Conversation
The original migration re-joined the hosts table on every batch of 1000 cert IDs, causing ~14 minutes of sustained full DB load at 100K hosts. Instead, collect all Windows host IDs once upfront and then soft-delete certs in batches by host_id without the repeated join.
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Optimizes the ReparseWindowsHostCertificates MySQL migration to avoid repeatedly joining hosts during batched soft-deletes, reducing DB load during large upgrades (e.g., 100K hosts).
Changes:
- Collect Windows host IDs once, then soft-delete
host_certificatesinhost_idbatches (instead of paging by certificate ID with repeatedhostsjoins). - Keep the existing migration test validating the intended soft-delete behavior.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/datastore/mysql/migrations/tables/20260723181402_ReparseWindowsHostCertificates.go | Refactors migration batching to reduce DB load by eliminating repeated hosts joins. |
| changes/50228-fix-reparse-certs-migration | User-visible changes entry (content excluded from review). |
Files excluded by content exclusion policy (1)
- changes/50228-fix-reparse-certs-migration
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+35
to
+38
| // softDeleteWindowsHostCertsForReparse collects Windows host IDs once upfront, | ||
| // then soft-deletes their certificate rows in batches using only the | ||
| // host_certificates primary key. This avoids re-joining the hosts table on | ||
| // every batch, which was causing ~14 minutes of sustained full DB load at |
Comment on lines
+73
to
76
| affected, _ := result.RowsAffected() | ||
| for range affected { | ||
| increment() | ||
| } |
Inserts 1,000 Windows hosts x 20 certs (20K rows) and verifies the migration completes in under 30s. Measured at 445ms with the fix, vs the original which took ~14 minutes at 100K hosts.
9 tasks
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/50228-fix-reparse-certs-migration
Comments suppressed due to low confidence (2)
server/datastore/mysql/migrations/tables/20260723181402_ReparseWindowsHostCertificates.go:76
- The progress loop is currently
for range affected, butaffectedis an int64 (rows affected) and you can’t range over an integer. This won’t compile and also drops the error from RowsAffected.
Consider handling the RowsAffected error and looping with an index so increment() is called once per affected row.
affected, _ := result.RowsAffected()
for range affected {
increment()
}
server/datastore/mysql/migrations/tables/20260723181402_ReparseWindowsHostCertificates.go:38
- The function comment says the batched delete uses only the
host_certificatesprimary key, but the implementation updates byhost_id IN (...)(which is the key optimization). Updating the comment will avoid confusion during future perf investigations.
// softDeleteWindowsHostCertsForReparse collects Windows host IDs once upfront,
// then soft-deletes their certificate rows in batches using only the
// host_certificates primary key. This avoids re-joining the hosts table on
// every batch, which was causing ~14 minutes of sustained full DB load at
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50262 +/- ##
==========================================
+ Coverage 68.08% 68.12% +0.03%
==========================================
Files 3936 3937 +1
Lines 250690 250978 +288
Branches 13239 13239
==========================================
+ Hits 170687 170977 +290
+ Misses 64692 64673 -19
- Partials 15311 15328 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Closes #50228
Checklist for submitter
changes/.SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
For unreleased bug fixes in a release candidate, one of:
Summary
The
ReparseWindowsHostCertificatesmigration (from #49787) causes ~14 minutes of sustained full DB load at 100K hosts during the 4.89 to 4.90 upgrade:Root cause: The original migration re-joins the
hoststable on every batch of 1,000 cert IDs to filter byplatform = 'windows'. At 100K hosts with many certs per host, this repeated JOIN is expensive.Fix: Collect all Windows host IDs in a single query upfront (bounded by host count, not cert count), then soft-delete certs in batches of 500 host IDs using
WHERE host_id IN (?) AND origin = 'osquery' AND deleted_at IS NULLwithout re-joininghosts.This should reduce the migration from ~14 minutes to under a minute.
Needs cherry-pick to 4.90 RC.
Test plan
TestUp_20260723181402passes (verifies correct soft-delete behavior)