Build/Test Tools: Relax redo log durability for the test database - #12767
Build/Test Tools: Relax redo log durability for the test database#12767adimoldovan wants to merge 4 commits into
Conversation
The mysql service sets no command and ships no my.cnf, so it runs with innodb_flush_log_at_trx_commit=1 and flushes the redo log to disk at every commit. The PHPUnit suite commits twice per test class, so the multisite suite pays about 1,622 redo-log fsyncs per run to protect a database that every job destroys. Value 2 removes those fsyncs and still writes the log at every commit, so only a kernel crash or power cut can lose data, not a mysqld crash. See #65762.
There was a problem hiding this comment.
🟡 Not ready to approve
The durability setting is hard-coded for all local-env uses (including persistent named volumes), so it should be made configurable (or clearly documented inline) to avoid unexpected data-loss risk for local contributors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adjusts the local Docker DB container configuration to reduce InnoDB redo-log flush durability during commits, aiming to speed up PHPUnit runs by avoiding per-commit fsync overhead in an ephemeral/CI-oriented database setup.
Changes:
- Add a
command:override to themysqlservice to setinnodb_flush_log_at_trx_commit=2.
File summaries
| File | Description |
|---|---|
| docker-compose.yml | Sets MySQL/MariaDB runtime flag to relax redo-log durability for faster test DB commits. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| image: ${LOCAL_DB_TYPE-mysql}:${LOCAL_DB_VERSION-latest} | ||
|
|
||
| command: [ "--innodb-flush-log-at-trx-commit=2" ] | ||
|
|
There was a problem hiding this comment.
🟡 Not ready to approve
The durability reduction applies to the default local development database (with a persistent named volume) and should be made configurable/opt-out to avoid unexpected local data-loss risk.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
docker-compose.yml:70
docker-compose.ymlis also the default local development environment configuration and the MySQL service uses a named volume (mysql:/var/lib/mysql), so the database is not always “disposable”. Loweringinnodb_flush_log_at_trx_commitcan increase the chance of losing up to ~1s of recent transactions on a host/kernel crash, which may surprise local contributors.
Consider making this setting configurable via an environment variable (defaulting to 2 for CI speedups) and documenting the override (e.g. in .env.example), so local setups can opt back into full durability.
# This database is disposable, so it does not need a durable write at every commit.
command: [ "--innodb-flush-log-at-trx-commit=2" ]
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…hanges. docker-compose.yml defines the environment the suite runs in, but the PHPUnit workflow does not list it in its path filter. A change to that environment therefore never runs the tests it affects. See #65762.
ffd69c9 to
a83302a
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The change is small, syntactically correct, and scoped to test/development tooling with a clear CI trigger update to match the behavior impact.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The new inline comment in docker-compose.yml is misleading because this compose setup is also used for persistent local dev DBs (named volume), so the comment should describe the durability tradeoff more accurately.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
docker-compose.yml:69
- The comment says the database is "disposable", but this compose file is also used for the local dev environment and persists data via the named
mysql:volume. Consider rewording to describe the tradeoff (faster tests vs reduced durability) and point to the override mechanism for anyone who needs fully durable commits.
# This database is disposable, so it does not need a durable write at every commit.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The compose change sets innodb_flush_log_at_trx_commit=0, which contradicts the PR description’s stated intent to use value 2 for the test database.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
docker-compose.yml:71
- The PR description explains why
innodb_flush_log_at_trx_commitshould be lowered to2(not0) to avoid unnecessary fsyncs while still writing the redo log to the OS cache at each commit. The compose change currently sets it to0, which contradicts the stated risk analysis and reduces crash-safety beyond what was described.
# This database is disposable, so it does not need a durable write at every commit.
command: [ "--innodb-flush-log-at-trx-commit=0" ]
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
CI shows no relevant improvement. Closing. |
The
mysqlservice indocker-compose.ymlsets nocommand:and ships nomy.cnf, so it runs withinnodb_flush_log_at_trx_commit=1and flushes the redo log to disk at every commit. That is the right default for a production database. Every job destroys this one.The PHPUnit suite commits twice per test class:
set_up_before_class()andtear_down_after_class()both callcommit_transaction(). Per-test transactions roll back and need no flush. The multisite suite runs 1,098 classes, so 2,196 commits per run.Replaying that commit pattern against MySQL 8.0 and reading
Innodb_os_log_fsyncsbefore and after:innodb_flush_log_at_trx_commitValue 2, not 0. Both remove the same flushes, but 2 still writes the log to the operating system cache at every commit, so a mysqld crash loses nothing and only a kernel crash or power cut can lose up to a second of commits. Value 0 gives up that guarantee and saves nothing measurable.
Eight full runs of the multisite suite, 31,315 tests each, three at the default and five with the setting lowered. Comparing every individual test outcome against the reference run, not just the totals: zero missing, zero new, zero changed, no new flakiness.
One
command:serves all twelve matrix combinations. mysql 5.7, 8.0, 8.4 and 9.7 and mariadb 5.5, 10.3, 10.11, 11.8 and 12.1 all accept the flag and report it back. mariadb 10.5, 10.6 and 11.4 fall between tested versions.Full reasoning, evidence and risk analysis are in the Trac ticket.
Trac ticket: https://core.trac.wordpress.org/ticket/65762
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Investigating the cause of the timeouts, running the benchmarks, and drafting the ticket and this change. I directed the work, reviewed the results and made the final decisions.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.