PMM-15195 Report the InnoDB redo log size on MySQL 8.0.30+ and 9.7 in pt-mysql-summary. - #1152
Conversation
Avoid requiring mysqldump for reports without schema analysis and use dynamic redo log capacity when legacy log variables are unavailable. Co-authored-by: Cursor <cursoragent@cursor.com>
Fail early when mysqldump is unavailable so Service Summary cannot silently omit schema-related information. Co-authored-by: Cursor <cursoragent@cursor.com>
This reverts commit ff936cd.
JiriCtvrtka
changed the title
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Updates pt-mysql-summary to better support MySQL 8.0.30+ / MySQL 9.7 InnoDB redo logging, while making mysqldump optional unless schema analysis is explicitly requested. Also aligns Go toolchain versions for generated binaries/package builds.
Changes:
- Report InnoDB “Log File Size” using
innodb_redo_log_capacitywhen available, with legacy fallback toinnodb_log_file_size * innodb_log_files_in_group. - Require
mysqldumponly when--databasesor--all-databasesschema analysis is requested; add test coverage for this behavior. - Bump Go version to
1.26.5and align the package builder script withgo.mod.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| t/pt-mysql-summary/samples/expected_output_temp_enc009.txt | Updates expected InnoDB log size output to match redo capacity reporting. |
| t/pt-mysql-summary/samples/expected_output_temp_enc008.txt | Same expected-output update for another sample fixture. |
| t/pt-mysql-summary/pt-mysql-summary.t | Adds tests ensuring mysqldump is optional for default summary but required for schema analysis. |
| t/lib/bash/report_mysql_info.sh | Adds a focused test validating formatting via innodb_redo_log_capacity; updates TAP plan. |
| lib/bash/report_mysql_info.sh | Implements redo-capacity-based “Log File Size” reporting with legacy fallback and “Unknown” fallback. |
| go.mod | Bumps the Go toolchain directive to 1.26.5. |
| config/scripts/pt_builder.sh | Aligns builder-installed Go version with go.mod. |
| bin/pt-mysql-summary | Mirrors redo-capacity logic in the shipped script and gates mysqldump validation on schema-analysis options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot review |
There was a problem hiding this comment.
Two inline comments below. Two points that don't anchor to the diff:
=head1 OUTPUT POD is now stale
bin/pt-mysql-summary:3136 still documents:
Log File Size | 2 * 5.0M = 10.0M
and the sample config just below at bin/pt-mysql-summary:3245 reinforces it with innodb_log_file_size = 5M. Nothing tells the reader that on 8.0.30+ this field switches from count * per-file = total to a bare total. "Documentation updated - no changes needed" is fair for =head1 SYSTEM REQUIREMENTS (it never mentioned mysqldump), but the OUTPUT section now describes only the legacy rendering. A one-sentence note there should head off some "why did my summary format change?" reports.
Changelog
Changelog isn't updated, and at least one user-visible item here arguably belongs in it: Log File Size output format change for MySQL 8.0.30+. Convention in this repo looks like release-time updates rather than per-PR, so if release prep picks these up, could you confirm both land in the next version? Otherwise a short entry in this PR would be good.
|
Hi @svetasmirnova, I saw that you opened this PR yesterday: #1155. My ticket on the PMM side is to add support for MySQL 9.7. To implement it fully, I also created a PR for Percona Toolkit with the necessary changes. What do you think would be the best approach at this point? Thank you. |
|
Hi @svetasmirnova could you check #1152 (comment)? If we can decide about next steps. |
JiriCtvrtka
changed the title
ademidoff
left a comment
ademidoff
left a comment
There was a problem hiding this comment.
Reviewed as the third leg of PMM-15195, alongside percona/mysqld_exporter#387 and percona/pmm#5648. Rather than reading the branch, I drove the new section_innodb code directly against lib/bash/report_mysql_info.sh and checked every claim against real servers.
This is the cleanest of the three legs. The logic is correct on every shape I could produce, the test data reflects real servers, and the Go bump is legitimate. One real defect — the man page and two code comments name the wrong MariaDB version, and they contradict this PR's own description. Details inline.
What I verified
Drove the new branch through eight shapes — your four plus four of my own:
| Shape | Output |
|---|---|
| 8.0.30 (all three vars) | 100.0M |
| 9.7 (capacity only) | 100.0M |
MariaDB 10.6 (log_file_size only) |
96.0M |
| MariaDB 10.5 (file count = 1) | 1 * 96.0M = 96.0M |
| nothing present | Unknown |
| capacity = 0, no legacy | Unknown |
| capacity non-numeric, legacy present | falls back to legacy |
| capacity = 0, legacy present | falls back to legacy |
All eight correct — the [ "${redo_capacity}" -gt 0 ] 2>/dev/null guard degrades the way it needs to.
- The changed expected outputs are a real fix, not a test fudge.
t/pt-mysql-summary/samples/temp_enc008/mysql-variablesandt/pt-mysql-summary/samples/temp_enc009/mysql-variablescarry all three variables (innodb_log_file_size 5242880,innodb_log_files_in_group 2,innodb_redo_log_capacity 104857600). On 8.0.30+ the legacy pair is deprecated and ignored, so the old2 * 5.0M = 10.0Mwas actively wrong and100.0Mis the capacity the server actually honours. Worth stating that explicitly in the description — as written, a reviewer could read those two sample changes as adjusting tests to match new behaviour. - The
check_mysqlgating is safe.OPT_ALL_DATABASESis a real option and the new guard matches the existing idiom atbin/pt-mysql-summary:1177andbin/pt-mysql-summary:2564.CMD_MYSQLDUMPis referenced only atbin/pt-mysql-summary:1081andbin/pt-mysql-summary:1091, both inside the schema-dump path gated on those same options, so no default-path use survives and the check can safely be conditional. - The
bin/vslib/comment divergence is expected, not hand-editing —util/update-modules:92pipes module content throughgrep -v '^ *#'. I checked because the two blocks differ; they differ correctly. go build ./...clean under Go 1.26.6, andbash -nclean onbin/pt-mysql-summary,lib/bash/report_mysql_info.shandconfig/scripts/pt_builder.sh.- CVE-2026-39822 checks out:
os.Rootimproperly follows symlinks outside the root on Unix when the final path component is a symlink and the path ends in/(CWE-61, CVSS 7.8), fixed in Go 1.25.12 / 1.26.5 / 1.27.0-rc.2. Both the bump and the citation are accurate.
Note for QA
This leg satisfies the ticket's Service Summary criterion differently than written. The AC asked for mysqldump to be available in the execution PATH of the service generating the report; this PR makes it optional instead, which is the better fix. Worth flagging on the ticket so QA doesn't go looking for a pmm-client packaging change that no longer needs to happen.
Thanks — all three inline items are fixed, and one of them turned out to be two bugs rather than one: the MySQL removal version was wrong as well as the MariaDB one. It's 9.3.0, measured across 9.0.1 / 9.1.0 / 9.2.0 (all three variables present) versus 9.3.0 / 9.4.0 / 9.7.0 (capacity only). Details in the threads. On the Go bump being unrelated scope — I'd push back on that one, because it turns out to be what the build gate requires rather than optional hardening. That job ends in a Trivy scan with severity: CRITICAL,HIGH, ignore-unfixed: true, exit-code: 1. I reproduced it locally against the same image (oraclelinux:9-slim + microdnf -y update + COPY bin/*) and got exit 1 with 10 HIGH findings, all Go and no OS: stdlib v1.26.5 — 8 HIGH, fixed in 1.26.6: encoding/asn1, x/net/idna, x/net/dns/dnsmessage, net/http, html/template, encoding/xml, net/url, crypto/tls On the Changelog: confirmed, the convention here is release-time only — neither Changelog nor docs/release_notes.rst has an unreleased section, and every commit touching them is release prep. The two items to pick up at release prep are the Log File Size output change on 8.0.30+ and mysqldump becoming optional for the default summary. Both are now called out in the description. On the QA note: agreed and added to the description, and I'll put it on the ticket so nobody chases a pmm-client packaging change. Still outstanding and not mine to settle: #1155 touches the same section_innodb hunk with incompatible behaviour, and I haven't had a reply from @svetasmirnova since 30 Jul or 20 Aug. That's the real blocker on this leg. |
Conflicts in go.mod and go.sum: 3.x picked up dependabot bumps for golang.org/x/crypto (0.54.0), golang.org/x/sys (0.47.0) and golang.org/x/text (0.40.0) while this branch raised the go directive to 1.26.7. Resolved by taking the newer of each: go 1.26.7 with x/crypto 0.54.0, x/net 0.56.0, x/sys 0.47.0 and x/text 0.40.0. 3.x had already fixed x/net and x/text independently, so the go directive is the only piece this branch still contributes to the build job's Trivy gate. go mod verify and go build ./... clean; go mod tidy is a no-op. Reproducing the build job's image and scan locally reports no findings and exits 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@JiriCtvrtka #1155 is not only about pt-mysql-sumary, it is about if the toolkit can work with 9.7. It is merged now, and I am reviewing this PR. pt-mysql-summary requires more changes, see https://perconadev.atlassian.net/browse/PT-2505 So this will be the first step in improving pt-mysql-summary work. |
PMM-15195
PMM PR: percona/pmm#5648
MySQL Exporter PR: percona/mysqld_exporter#387
FB: Percona-Lab/pmm-submodules#4479
Summary
Updates
pt-mysql-summaryfor MySQL 9.7 and removes an unnecessary historicaldependency on
mysqldump.Log File Sizefrominnodb_redo_log_capacityon MySQL8.0.30+, which is the value the server honours there even while the legacy
variables are still present.
innodb_log_file_sizeandinnodb_log_files_in_groupwere deprecated in 8.0.30 and removed in 9.3.0 —measured: 9.0.1, 9.1.0 and 9.2.0 still expose all three variables, while
9.3.0, 9.4.0 and 9.7.0 expose only the capacity.
innodb_log_files_in_group * innodb_log_file_sizerendering for older MySQL versions.
innodb_log_file_sizeis exposed (MariaDB 10.6+, which removedinnodb_log_files_in_groupand never hadinnodb_redo_log_capacity), reportsthat size as a bare total rather than discarding it as
Unknown, matching howthe redo capacity is rendered on 8.0.30+. MariaDB 10.5 still exposes the file
count with value 1 — it was deprecated and ignored in 10.5.2 and only removed
in 10.6.0 — so there the legacy
1 * 96.0M = 96.0Mrendering still applies.Measured on 10.5.29 and 10.6.28.
Unknownonly when no redo size variable of any spelling ispresent. A capacity that is present but zero or non-numeric degrades to the
legacy variables rather than being reported or turning into
Unknown.mysqldumponly when schema analysis is explicitly requested with--databasesor--all-databases; the default summary no longer fails whenmysqldumpis missing.=head1 OUTPUTthat theLog File Sizerendering is versiondependent, naming the versions where each form applies, so the change from
count * per-file = totalto a bare total on 8.0.30+ is not a surprise.aligns
config/scripts/pt_builder.shwith the version declared ingo.mod.Compatibility
Default
pt-mysql-summaryand PMM Service Summary reports carry the sameinformation as before. On MySQL 8.0.30+ the
Log File Sizeline prints the totalredo log capacity instead of
files * size = total, because that capacity iswhat the server honours from 8.0.30 on — and from 9.3.0 the per-file variables
are gone entirely. Schema analysis still requires
mysqldump.Note for QA
This satisfies the ticket's Service Summary criterion differently than written.
The AC asked for
mysqldumpto be available in the execution PATH of the servicegenerating the report; this PR makes
mysqldumpoptional for the default summaryinstead, so no pmm-client packaging change is needed.
Test plan
cases in total (
plan51 → 54): the 8.0.30 shape (all three variablespresent, capacity wins), the 9.3+ shape (capacity only), the MariaDB 10.6
shape (
innodb_log_file_sizeonly), the MariaDB 10.5 shape (file count ofone, which keeps the legacy rendering), the
Unknownbranch, and the twodegradation paths where
innodb_redo_log_capacityis present but zero ornon-numeric.
[ "${redo_capacity}" -gt 0 ] 2>/dev/null, which relies on[returning anon-zero status for a non-numeric operand with the error text suppressed.
That is correct in bash, but it is the kind of idiom whose behaviour drifts
between shells, so it is pinned by tests rather than by a comment.
section_innodbdirectly against thetemp001sample:all seven shapes produce the asserted output.
t/pt-mysql-summary/samples/expected_output_temp_enc008.txtand..._enc009.txtare a fix, not a test adjustment. Both samples carry all threevariables (
innodb_log_file_size 5242880,innodb_log_files_in_group 2,innodb_redo_log_capacity 104857600). On 8.0.30+ the legacy pair is deprecatedand ignored, so the old
2 * 5.0M = 10.0Mreported a size the server does nothonour;
100.0Mis the capacity actually in effect.mysqldumpis optional for the default summary andrequired for
--databases/--all-databases.MySQL 9.7.
mysqldumpbinary can connect to MySQL 9.7.package builder shell syntax.
bash -nclean onbin/pt-mysql-summary,lib/bash/report_mysql_info.shandconfig/scripts/pt_builder.sh.Changelog
Not updated here: the convention in this repo is release-time only — neither
Changelognordocs/release_notes.rsthas an unreleased section, and everycommit touching them is release preparation. Two user-visible items to pick up
at release prep:
Log File Sizeoutput format change on MySQL 8.0.30+.mysqldumpis no longer required for the defaultpt-mysql-summaryrun.