-
Notifications
You must be signed in to change notification settings - Fork 4k
sql/schemachanger: improve performance of TestBackupSuccess #152112
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
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
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.
Pull Request Overview
This PR refactors the backup success tests for the declarative schema changer to improve performance by ~30%. Instead of starting a server for each test stage and running the full schema change every time, it creates a single server once, takes all necessary backups during the schema change execution, and then reuses those backups across test cases.
- Introduces a shared server pattern for backup tests with a new
TestServer
struct - Refactors
backupSuccess
into preparation and execution phases to enable backup reuse - Updates the test framework to support optional preparation functions
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
pkg/sql/schemachanger/sctest/test_server_factory.go |
Adds TestServer struct and Start method to enable server reuse across test cases |
pkg/sql/schemachanger/sctest/framework.go |
Updates framework to accept optional preparation function for test setup |
pkg/sql/schemachanger/sctest/cumulative.go |
Updates test calls to pass nil for preparation function where not needed |
pkg/sql/schemachanger/sctest/backup.go |
Refactors backup tests to use preparation phase for taking backups and execution phase for testing |
pkg/ccl/schemachangerccl/multiregion_testcluster_factory.go |
Implements Start method for multi-region test cluster factory |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
key.Phase, key.Ordinal) | ||
backupStmt := fmt.Sprintf("BACKUP DATABASE %s INTO '%s'", dbName, url) | ||
_, err := dbForBackup.Load().Exec(backupStmt) | ||
if err != nil { |
Copilot
AI
Aug 19, 2025
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.
The error check is missing a return statement. If the backup fails, the function should return the error instead of continuing to update the backups map.
Copilot uses AI. Check for mistakes.
b1042ac
to
2c014b4
Compare
Previously, we only supported running a closure against a TestServer in the schema changer testing framework. This was problematic for tests that could save time by sharing servers (for example BACKUP / RESTORE). To address this, this patch introduces a Start method for starting up a server. Release note: None
d95c1f1
to
c16afcf
Compare
c16afcf
to
1085591
Compare
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.
nice work!
Previously, the successful backup tests would for each stage of a schema change would run the full schema change, taking a backup at the target stage. This patch modifies the TestBackupSuccess variants of the schema changer test to start the server once, take all the backups and then restore the stages we should be testing. Testing show around a ~30% improvement with this change in execution time. Fixes: cockroachdb#152076 Release note: None
1085591
to
3af68bd
Compare
@rafiss TFTR bors r+ |
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #152076: branch-release-25.2, branch-release-25.3, branch-release-25.4. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 09a15e4 to blathers/backport-release-25.2-152112: POST https://api.github.com/repos/fqazi/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 25.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Previously, the successful backup tests would for each stage of a schema
change would run the full schema change, taking a backup at the target
stage. This patch modifies the TestBackupSuccess variants of the schema
changer test to start the server once, take all the backups and then
restore the stages we should be testing.
Testing show around a ~30% improvement with this change in execution
time.
Fixes: #152076
Release note: None