Repository navigation
feat(db): add db:mysql-optimize (beta), to replace db:size --cleanup - #290
Closed
pjcdawkins wants to merge 6 commits into
Closed
pjcdawkins wants to merge 6 commits into
pjcdawkins wants to merge 6 commits into
Conversation
A prototype for #124: a MySQL/MariaDB-specific command that lists InnoDB tables whose rebuild would reclaim disk space, and rebuilds them only with --execute. - Reads the server's settings over SQL: version, innodb_file_per_table, wsrep_on and wsrep_cluster_size (Galera), tmpdir and innodb_tmpdir. On MySQL 8 it sets information_schema_stats_expiry = 0 so statistics are fresh; --analyze runs ANALYZE TABLE on the tables found. - Lists tables with at least --min-reclaimable (100M) and --min-percent (20%) of their size reclaimable, and whether each rebuild fits in the service's disk and /tmp (from the metrics API), leaving 10% free. - Flags tables that are probably in a shared tablespace (same DATA_FREE as another table, or innodb_file_per_table OFF), and tables with FULLTEXT or SPATIAL indexes, which cannot be rebuilt online. - --execute runs ALTER TABLE ... FORCE, ALGORITHM=INPLACE, LOCK=NONE (the same rebuild as OPTIMIZE TABLE, but it fails instead of locking), with escaped identifiers. It refuses when the headroom is unknown or too small, or on a Galera cluster, unless --force. - db:size --cleanup still works, with a deprecation notice, and now escapes identifiers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnUBGycBKd23tLPMcyf4Rb
DATA_FREE only counts free extents, so it misses space freed inside pages, e.g. after scattered deletes or updates to shorter values. On MySQL 8.0 and MariaDB 11.4, a table with 9 of 10 rows deleted reported 5 MiB in DATA_FREE, and a rebuild freed 62-67 MiB. db:mysql-optimize now samples about 1000 rows of each table that could reach --min-reclaimable, and estimates the rebuilt size: - Row bytes are measured with LENGTH() for variable-size columns, with fixed sizes for other types and 20 bytes per row for InnoDB headers. - With a single-column integer primary key, rows are read from 50 ranges of 20 rows, starting at uneven intervals of the key (a golden ratio sequence), so that they do not align with a pattern that repeats in the data. Each range is an index lookup. Otherwise, the first 1000 rows are read, and the table is noted as such. - The rebuilt data size is the row bytes x TABLE_ROWS x 1.5. Secondary indexes are scaled by the same ratio as the data, capped at their current size. - The reclaimable space is the larger of DATA_FREE and the current size (with free extents) minus the rebuilt size. - The disk needed for a rebuild is the rebuilt size, plus 10% and 8 MiB, instead of the table's current size. The thresholds are now applied after sampling. Tables in a shared tablespace are not sampled. If the sample cannot be read, only DATA_FREE is used, with a warning. Checked against real servers with a user with privileges on its database only: the estimates were within 8 MiB of the space freed by rebuilds of six test tables, and the disk needed was above the size of each rebuilt file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![upsun-dispatch[bot]](https://avatars.githubusercontent.com/in/4153575?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 3 warnings · 🔵 2 minor points
🔍 Full review · 8 files reviewed
Verification
sampleRowskeeps the original keys of$tablesthrougharray_filter, so the sample index written into each query maps back to the rightTableCandidate.- Identifiers in ALTER/ANALYZE/sample queries go through
quoteIdentifier, and string literals in the table/column filters go throughquoteString; both handle backticks, quotes and backslashes. latestServiceMetricsasks for the same types, aggregations and/mnt//tmpmountpoints that the existingmetrics:allcommand reads, includingdisk_limit.max.- Without
--yes,QuestionHelper::confirmreturns its default (true) in non-interactive mode, so--executeruns non-interactively once the safety checks pass, as the integration tests expect.
New PHPUnit tests (legacy/tests/Model/Mysql/MysqlOptimizeTest.php, run by the CI legacy-php job) cover parsing, quoting, the estimates and query building. Integration tests (integration-tests/db_mysql_optimize_test.go, run by the integration-test and integration-test-legacy-auth jobs) drive the command against a fake mariadb script. No test covers a failed rebuild's error output, stale TABLE_ROWS, or shared-tablespace tables below the threshold.
Review details
- Commit: 9128d6e
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
- Show only the database's error when a rebuild fails. The exception's message contains the command line, with the database password, and on remote hosts it leaves out the error output. - Count tables as probably in a shared tablespace when they share at least 8 MiB of free space, instead of --min-reclaimable. With row sampling, tables with less free space than the threshold can be candidates, and they were never flagged. Tested with two tables created while innodb_file_per_table was OFF on MySQL 8.0 and MariaDB 11.4: both report 18 MiB, and are now flagged. - Do not use the sample if TABLE_ROWS is lower than the number of rows sampled: statistics are out of date, and the estimate (proportional to TABLE_ROWS) could let a rebuild through the disk check. - With --analyze, analyze every table checked, before sampling, and sample once. MariaDB's default ANALYZE TABLE does not scan tables for column statistics (use_stat_tables=PREFERABLY_FOR_QUERIES). - Show the deprecation notice of db:size --cleanup for any database. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Size column was the data and indexes only, so the reclaimable space (which includes DATA_FREE) could be more than the size, and the reclaimable percentage more than 100%. Size now includes DATA_FREE (about the size of the table's file), and --min-percent is relative to it. Without a row sample, the disk needed for a rebuild is still the data and indexes, as free extents are not copied. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning
🔁 Incremental (head + base moved) · 4 files reviewed
Verification
reclaimable()now computessize() - rebuiltwithout addingdataFreea second time, so the double count from the earlier version is gone.- The unsampled
diskNeeded()returnsdataLength + indexLength, which keeps its earlier value now thatsize()includes free extents (the unit test still asserts 100 MiB). - The integration expectations agree with the new formula: for example, cache 4294967296 = 3 GiB + 1 GiB free, and 1073741824/4294967296 is 25%.
tablesQueryalready filtered onDATA_LENGTH + INDEX_LENGTH + DATA_FREE, so the SQL pre-filter now matchessize().
MysqlOptimizeTest::testEstimates now asserts the new size(). The CSV and table expectations in TestDbMysqlOptimizeReport were updated for it. No test covers how size looks for a table in a shared tablespace.
Review 3 of 10 for this pull request · View the full run
In a shared tablespace, DATA_FREE is the free space of the whole tablespace, reported on every table in it. Adding it to each table's size made small tables look as large as the tablespace's free space. For tables that are probably in a shared tablespace, the size is now the data and indexes only, and the reclaimable percentage is left empty, as a rebuild would not reclaim that space. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
@pjcdawkins Are you sorting the tables from small -> big when running the optimize? When mariadb/mysql does a recreate of the table, it will need to copy the data to a new table. So momentarily it needs additional space. If you start with the biggest table, you might not be able to run the recreate due to running out of disk space. |
Tables were rebuilt in order of reclaimable space, largest first. A rebuild copies the table, so on a nearly full disk the largest rebuild is the most likely to fail. Rebuilding the tables that need the least disk space first frees space for the larger ones, as db:size --cleanup did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@matthiaz good point, that's now what the PR does (initially it did largest first) |
Contributor
Author
|
Proposing upsun/ai#42 instead of this PR |
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.
Adds
db:mysql-optimize(beta), for MySQL and MariaDB. It finds InnoDB tables with reclaimable disk space and, with--execute, rebuilds them online. It replacesdb:size --cleanup, which still works but now prints a deprecation notice, and escapes identifiers.This is a prototype, so the command may not belong in the CLI.
Closes #124
What it does
SHOW GLOBAL VARIABLES/STATUS), and the disk and/tmpusage of the service from the metrics API.DATA_FREE) and from a sample of about 1000 rows.DATA_FREEalone misses space freed inside pages: on a test table with 9 of 10 rows deleted, it reported 5 MiB, while a rebuild freed 62–67 MiB.TABLE_ROWS× 1.5. Indexes are scaled by the same ratio as the data, never above their current size.--min-reclaimable(default 100M) and--min-percent(default 20%), with whether the rebuild fits on the disk and in/tmp. The estimate leaves 10% of each volume free, and adds 10% and 8 MiB to the rebuilt size.--executerunsALTER TABLE ... FORCE, ALGORITHM=INPLACE, LOCK=NONE, which fails rather than locking the table. It refuses when the headroom is unknown or too small, or on a Galera cluster of more than one node, unless--forceis given.--analyzerunsANALYZE TABLEfirst, to refresh statistics.Verification
make lint,make test, and the integration tests pass. The db tests also pass withTEST_CLI_GO_AUTH=0. The integration tests use a fakemariadbclient and mocked metrics.Known limits
TABLE_ROWSis approximate, a sample of the first rows can be biased, and the 1.5 factor was measured on the test tables (1.3–1.45), not derived./tmpestimate doesn't count the log of concurrent writes during a rebuild (up toinnodb_online_alter_log_max_size)./tmpthat the metrics measure.🤖 Generated with Claude Code