br: compare collate check privilege - #70258
Conversation
Signed-off-by: Jianjun Liao <jianjun.liao@outlook.com>
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
release-note-none
📝 WalkthroughWalkthroughThe restore flow now separates type and collation compatibility. It validates selected MySQL privilege tables and row counts before replacement, allowing supported ChangesPrivilege table restore compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RestoreTask
participant Client
participant PrivilegeCompatibilityCheck
participant PrivilegeTableSQL
RestoreTask->>Client: check system-table compatibility
Client->>PrivilegeCompatibilityCheck: validate privilege-table metadata
PrivilegeCompatibilityCheck->>PrivilegeTableSQL: query upstream and downstream row counts
PrivilegeTableSQL-->>PrivilegeCompatibilityCheck: return row counts
PrivilegeCompatibilityCheck-->>Client: return compatibility result
Client->>Client: execute REPLACE INTO when compatible
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
br/pkg/restore/systable_restore.go (2)
318-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the collation-compatibility contract on these helpers.
checkSysTableColumnCollateCompatibilityandcheckPrivilegeTableRowsCollateCompatibilityimplement a non-obvious contract: only an upstreamutf8mb4_binto downstreamutf8mb4_general_citransition is accepted, and only for the columns listed incollateCompatibilityTables. Add a short doc comment on each function explaining this constraint and why it exists (v6.5-to-v7.2+ default collation change for privilege tables). This helps future readers avoid accidentally widening or narrowing the supported transition without understanding the compatibility guarantee it encodes.As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs."
Also applies to: 335-379
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/restore/systable_restore.go` around lines 318 - 333, Add concise doc comments to checkSysTableColumnCollateCompatibility and checkPrivilegeTableRowsCollateCompatibility describing that they accept only utf8mb4_bin-to-utf8mb4_general_ci transitions for columns listed in collateCompatibilityTables, preserving the v6.5-to-v7.2+ privilege-table default-collation compatibility contract.Source: Coding guidelines
64-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hardcoding the temporary database name; add a comment for the compatibility contract.
The literal
__TiDB_BR_Temporary_mysqlappears six times in these SQL strings. Elsewhere in this package (for exampleHasBackedUpSysDB), the temporary database name is derived fromutils.TemporaryDBName(mysql.SystemDB). Build the SQL from that helper instead of a raw literal, so the queries stay correct if the temporary-database naming convention changes.Also add a short comment above
collateCompatibilityTablesexplaining the contract: which tables/columns support a collation change, and why onlydb,tables_priv, andcolumns_priv(and only specific columns within them) are listed.As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs."
♻️ Proposed direction
+var mysqlTemporaryDBName = utils.TemporaryDBName(mysql.SystemDB).L + +// collateCompatibilityTables lists privilege tables/columns for which a +// utf8mb4_bin (upstream) -> utf8mb4_general_ci (downstream) collation +// change is considered safe, provided the backed-up rows remain distinct +// under utf8mb4_general_ci. var collateCompatibilityTables = map[string]map[string]checkPrivilegeTableRowsCollateCompatibilitySQLPair{ "mysql": { "db": { - upstreamCollateSQL: "SELECT COUNT(1) FROM __TiDB_BR_Temporary_mysql.db", - downstreamCollateSQL: "SELECT COUNT(1) FROM (SELECT Host, DB COLLATE utf8mb4_general_ci, User FROM __TiDB_BR_Temporary_mysql.db GROUP BY Host, DB COLLATE utf8mb4_general_ci, User) as a", + upstreamCollateSQL: fmt.Sprintf("SELECT COUNT(1) FROM %s.db", mysqlTemporaryDBName), + downstreamCollateSQL: fmt.Sprintf("SELECT COUNT(1) FROM (SELECT Host, DB COLLATE utf8mb4_general_ci, User FROM %s.db GROUP BY Host, DB COLLATE utf8mb4_general_ci, User) as a", mysqlTemporaryDBName), columns: map[string]struct{}{"db": {}}, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/restore/systable_restore.go` around lines 64 - 89, Update collateCompatibilityTables to construct each temporary-database reference using utils.TemporaryDBName(mysql.SystemDB) instead of hardcoding __TiDB_BR_Temporary_mysql, while preserving the existing SQL and table mappings. Add a concise comment immediately above the variable describing that only db, tables_priv, and columns_priv support the collation-change compatibility check, with only their listed columns included, and explain the compatibility constraint behind this limited scope.Source: Coding guidelines
br/pkg/restore/client.go (1)
209-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a short comment for the new cross-file flag.
privilegeTableRowsCollateCompatibilityis set inclient.goand read insystable_restore.goto gate an extra validation step. Add a one-line comment above the field. Explain that it means "at least one privilege-table column has a supported utf8mb4_bin→utf8mb4_general_ci collation change" and thatreplaceTemporaryTableToSystableuses it to decide when to run the row-collision check.As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/restore/client.go` around lines 209 - 210, Add a one-line comment directly above privilegeTableRowsCollateCompatibility explaining that it indicates at least one privilege-table column supports the utf8mb4_bin→utf8mb4_general_ci collation change and that replaceTemporaryTableToSystable uses it to trigger row-collision validation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@br/pkg/restore/client.go`:
- Around line 209-210: Add a one-line comment directly above
privilegeTableRowsCollateCompatibility explaining that it indicates at least one
privilege-table column supports the utf8mb4_bin→utf8mb4_general_ci collation
change and that replaceTemporaryTableToSystable uses it to trigger row-collision
validation.
In `@br/pkg/restore/systable_restore.go`:
- Around line 318-333: Add concise doc comments to
checkSysTableColumnCollateCompatibility and
checkPrivilegeTableRowsCollateCompatibility describing that they accept only
utf8mb4_bin-to-utf8mb4_general_ci transitions for columns listed in
collateCompatibilityTables, preserving the v6.5-to-v7.2+ privilege-table
default-collation compatibility contract.
- Around line 64-89: Update collateCompatibilityTables to construct each
temporary-database reference using utils.TemporaryDBName(mysql.SystemDB) instead
of hardcoding __TiDB_BR_Temporary_mysql, while preserving the existing SQL and
table mappings. Add a concise comment immediately above the variable describing
that only db, tables_priv, and columns_priv support the collation-change
compatibility check, with only their listed columns included, and explain the
compatibility constraint behind this limited scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9633ee5a-87e3-4062-9030-4cb777e6e108
📒 Files selected for processing (9)
br/pkg/restore/BUILD.bazelbr/pkg/restore/client.gobr/pkg/restore/client_test.gobr/pkg/restore/systable_restore.gobr/pkg/restore/systable_restore_test.gobr/pkg/restore/util_test.gobr/pkg/task/restore.gobr/pkg/utils/misc.gobr/pkg/utils/misc_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.1 #70258 +/- ##
================================================
Coverage ? 37.0692%
================================================
Files ? 1510
Lines ? 577508
Branches ? 0
================================================
Hits ? 214078
Misses ? 342043
Partials ? 21387
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
@Leavrth: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: YuJuncen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
2 participants
What problem does this PR solve?
Issue Number: close #64667
Problem Summary:
It is needed to restore privileges tables backed up from v6.5 to newly created v7.2+ clusters.
What changed and how does it work?
permit to restore privileges tables from v6.5 to v7.2+ if all the data have the same behavior in
utf8mb4_binandutf8mb4_general_ci.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit