Repository navigation
br: support restore privileges tables from v6.5 to v7.5 (#64668) - #70432
ti-chi-bot wants to merge 1 commit into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@Leavrth This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
📝 WalkthroughWalkthroughAdds a ChangesSnapshot restore client
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RestoreTask
participant SnapClient
participant TargetSQL
participant PrivilegeNotifier
RestoreTask->>SnapClient: check system-table compatibility
SnapClient->>TargetSQL: inspect schemas and privilege rows
SnapClient->>TargetSQL: replace or rename system tables
SnapClient->>PrivilegeNotifier: flush updated privileges
SnapClient-->>RestoreTask: complete system-schema restore
Possibly related PRs
Suggested labels: Suggested reviewers: 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
@ti-chi-bot: The following tests 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
br/pkg/restore/snap_client/systable_restore_test.go (1)
110-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the commented-out line and clone consistently.
Line 111 is dead code. Line 112 appends to
dbTI.Columnsand assigns the result tomockedDBTI.Columns, which mixes the two table infos. UsemockedDBTI.Columnson both sides to keep the clone independent.♻️ Proposed cleanup
mockedDBTI := dbTI.Clone() - //dbTI.Columns = append(dbTI.Columns, &model.ColumnInfo{Name: ast.NewCIStr("new-name")}) - mockedDBTI.Columns = append(dbTI.Columns, &model.ColumnInfo{Name: ast.NewCIStr("new-name")}) + mockedDBTI.Columns = append(mockedDBTI.Columns, &model.ColumnInfo{Name: ast.NewCIStr("new-name")})🤖 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/snap_client/systable_restore_test.go` around lines 110 - 112, In the test setup around mockedDBTI, remove the commented-out append and update the active append to use mockedDBTI.Columns as both the source and destination, preserving the cloned dbTI independently.br/pkg/utils/misc.go (1)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a doc comment that explains the two results.
IsTypeCompatibleis exported and now returns two booleans with different meanings.typeEqcovers flags, evaluation type, length, decimal, elements, and charset.collateEqcovers only the collation. A caller cannot infer this split from the signature.📝 Proposed comment
+// IsTypeCompatible reports whether the target field type can hold the source +// field type. typeEq covers nullability, sign, evaluation type, length, +// decimal, enum/set elements, and charset. collateEq reports collation +// equality separately, so callers can accept a collation-only difference. func IsTypeCompatible(src types.FieldType, target types.FieldType) (typeEq, collateEq bool) {As per coding guidelines: "keep exported-symbol doc comments".
🤖 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/utils/misc.go` around lines 60 - 61, Add a Go doc comment directly above the exported IsTypeCompatible function explaining that typeEq compares flags, evaluation type, length, decimal, elements, and charset, while collateEq compares only collation.Source: Coding guidelines
br/pkg/task/restore.go (1)
285-286: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the risk of
sys-check-collationin the flag help text.The flag permits a restore that would otherwise be rejected. The current text describes the mechanism but not the consequence. State that BR aborts the restore if the privilege rows collide under
utf8mb4_general_ci, so operators understand the failure mode before they enable the flag.Also confirm whether this flag should stay visible or use
flags.MarkHidden, as other compatibility flags inDefineRestoreCommonFlagsdo.🤖 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/task/restore.go` around lines 285 - 286, Update the FlagSysCheckCollation help text in DefineRestoreCommonFlags to state that BR aborts the restore when privilege rows collide under utf8mb4_general_ci, in addition to describing the collation conversion. Review the surrounding compatibility flags and apply flags.MarkHidden if this flag should follow their visibility convention.br/pkg/restore/snap_client/systable_restore.go (1)
154-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving the temporary schema name instead of hardcoding it.
The SQL strings embed
__TiDB_BR_Temporary_mysql. The rest of the package builds this name withutils.TemporaryDBName(dbName). If the prefix changes, these literals silently break the check because the SQL then targets a missing table.Building the SQL from
utils.TemporaryDBNameat call time keeps one source of truth.🤖 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/snap_client/systable_restore.go` around lines 154 - 172, Update the collate compatibility SQL construction around collateCompatibilityTables to derive the temporary schema with utils.TemporaryDBName(dbName) at call time instead of embedding __TiDB_BR_Temporary_mysql in static literals. Preserve the existing queries and column mappings while ensuring all referenced tables use the generated schema name.br/pkg/restore/snap_client/export_test.go (1)
84-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
AllocTableIDsruns twice with the same arguments.Line 90 and line 99 make the same call. The first call also discards its return value. Remove the duplicate call.
♻️ Proposed cleanup
rc.dom = dom - rc.AllocTableIDs(context.TODO(), tables, false, false, nil) rewriteRules := &restoreutils.RewriteRules{ Data: make([]*import_sstpb.RewriteRule, 0), }Confirm that
AllocTableIDsreturns no error that the helper must handle.🤖 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/snap_client/export_test.go` around lines 84 - 100, Remove the redundant first AllocTableIDs call in CreateTablesTest, keeping the later invocation before CreateTables. Confirm AllocTableIDs has no error result requiring handling, and preserve the existing table-mapping and creation flow.
🤖 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.
Inline comments:
In `@br/pkg/restore/snap_client/client.go`:
- Around line 276-286: Resolve the merge conflicts in restore.go by removing all
unresolved conflict markers at the specified sections and retaining the correct
merged implementation for each block. Preserve the existing restore-flag wiring
through SnapClient.SetCheckPrivilegeTableRowsCollateCompatiblity and
GetCheckPrivilegeTableRowsCollateCompatiblity, ensuring the file compiles.
In `@br/pkg/restore/snap_client/export_test.go`:
- Around line 65-73: Update the callback registration in the test setup so the
close callback is appended to closeCallBacks rather than createCallBacks. Keep
createCallBacks containing only the rate-limit initialization callback, ensuring
close execution invokes only the callback that resets the rate limit.
In `@br/pkg/restore/snap_client/systable_restore_test.go`:
- Around line 137-145: Update the type-mismatch setup in the test around
CheckSysTableCompatibility to mutate the same table info that is passed to it:
change the relevant column on mockedDBTI rather than mockedUserTI. Preserve the
expected no-error assertion for this case, confirming the collate difference
remains skipped while the DB column type mismatch is exercised.
- Around line 395-397: Update TestMonitorTheSystemTableIncremental to assert
session.CurrentBootstrapVersion equals the release-8.1 branch value version199
instead of 253.
In `@br/pkg/restore/snap_client/systable_restore.go`:
- Around line 807-811: Update the error message in the colCount validation
branch to report utf8mb4_general_ci instead of utf8mb4_bin, keeping the existing
error type, formatting, and control flow unchanged.
In `@br/pkg/task/restore.go`:
- Around line 279-287: Resolve every merge conflict marker in
br/pkg/task/restore.go, including the blocks around flag registration, restore
client setup, and physical system-table loading. In the restore setup, retain
restore.CheckKeyspaceBREnable, client.LoadRestoreStores(ctx), and
client.SetConcurrency(uint(cfg.Concurrency)) together with SetRewriteMode(ctx)
and SetCheckPrivilegeTableRowsCollateCompatiblity(cfg.SysCheckCollation),
confirming SnapClient.SetRewriteMode exists. Ensure the retained branch declares
canLoadSysTablePhysical and err before the subsequent error handling and
fallback, and preserve or intentionally remove the code hidden near line 828.
In `@br/pkg/utils/misc_test.go`:
- Around line 81-83: Update the test case around IsTypeCompatible so the target
column’s length is explicitly smaller than the source length, rather than
assigning 99 to its flag field. Preserve the existing assertions and ensure the
inputs reach the length-comparison branch instead of returning early on flag
incompatibility.
In `@br/pkg/utils/misc.go`:
- Around line 60-70: Update the IsTypeCompatible call in the restore client
type-checking logic to receive both returned values, using the typeEq result for
the existing compatibility decision and handling collateEq as required by the
surrounding logic. Ensure the call no longer treats the two-value return as a
single boolean and compiles successfully.
---
Nitpick comments:
In `@br/pkg/restore/snap_client/export_test.go`:
- Around line 84-100: Remove the redundant first AllocTableIDs call in
CreateTablesTest, keeping the later invocation before CreateTables. Confirm
AllocTableIDs has no error result requiring handling, and preserve the existing
table-mapping and creation flow.
In `@br/pkg/restore/snap_client/systable_restore_test.go`:
- Around line 110-112: In the test setup around mockedDBTI, remove the
commented-out append and update the active append to use mockedDBTI.Columns as
both the source and destination, preserving the cloned dbTI independently.
In `@br/pkg/restore/snap_client/systable_restore.go`:
- Around line 154-172: Update the collate compatibility SQL construction around
collateCompatibilityTables to derive the temporary schema with
utils.TemporaryDBName(dbName) at call time instead of embedding
__TiDB_BR_Temporary_mysql in static literals. Preserve the existing queries and
column mappings while ensuring all referenced tables use the generated schema
name.
In `@br/pkg/task/restore.go`:
- Around line 285-286: Update the FlagSysCheckCollation help text in
DefineRestoreCommonFlags to state that BR aborts the restore when privilege rows
collide under utf8mb4_general_ci, in addition to describing the collation
conversion. Review the surrounding compatibility flags and apply
flags.MarkHidden if this flag should follow their visibility convention.
In `@br/pkg/utils/misc.go`:
- Around line 60-61: Add a Go doc comment directly above the exported
IsTypeCompatible function explaining that typeEq compares flags, evaluation
type, length, decimal, elements, and charset, while collateEq compares only
collation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e792b7d8-2187-4f50-b9dd-522419903048
📒 Files selected for processing (8)
br/pkg/restore/snap_client/BUILD.bazelbr/pkg/restore/snap_client/client.gobr/pkg/restore/snap_client/export_test.gobr/pkg/restore/snap_client/systable_restore.gobr/pkg/restore/snap_client/systable_restore_test.gobr/pkg/task/restore.gobr/pkg/utils/misc.gobr/pkg/utils/misc_test.go
| // SetCheckPrivilegeTableRowsCollateCompatiblity set switch to check | ||
| // privilege tables with different collate columns | ||
| func (rc *SnapClient) SetCheckPrivilegeTableRowsCollateCompatiblity(v bool) { | ||
| rc.checkPrivilegeTableRowsCollateCompatiblity = v | ||
| } | ||
|
|
||
| // GetCheckPrivilegeTableRowsCollateCompatiblity get switch to check | ||
| // privilege tables with different collate columns | ||
| func (rc *SnapClient) GetCheckPrivilegeTableRowsCollateCompatiblity() bool { | ||
| return rc.checkPrivilegeTableRowsCollateCompatiblity | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the setter and getter for the privilege-table collation flag are wired across the codebase.
# Find every reference to the setter, getter, and backing field.
rg -nP -C4 '(Set|Get)?CheckPrivilegeTableRowsCollateCompatiblity' --type=go
# Confirm the systable restore path consumes the getter near the privilege-table check.
fd -t f 'systable_restore.go' --exec rg -nP -C6 'CollateCompatiblity|REPLACE INTO|utf8mb4_general_ci|utf8mb4_bin' {}
# Confirm the task layer sets the flag from the restore config.
fd -t f 'restore.go' -p 'br/pkg/task' --exec rg -nP -C6 'CollateCompatiblity' {}Repository: pingcap/tidb
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(client|systable_restore|restore)\.go$' | head -80
printf '%s\n' '--- all compatibility spellings and related symbols ---'
rg -n -P -C3 'CheckPrivilegeTableRowsCollate(?:Compatiblity|Compatibility)|CollateCompatiblity|CollateCompatibility' --glob '*.go' . || true
printf '%s\n' '--- privilege-table restore references ---'
rg -n -P -C5 'privilege|Privilege|collate|Collate' br/pkg/restore/snap_client br/pkg/task/restore.go --glob '*.go' | head -300 || trueRepository: pingcap/tidb
Length of output: 43562
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("br/pkg/restore/snap_client/client.go"),
Path("br/pkg/restore/snap_client/systable_restore.go"),
Path("br/pkg/task/restore.go"),
]
symbols = [
"SetCheckPrivilegeTableRowsCollateCompatiblity",
"GetCheckPrivilegeTableRowsCollateCompatiblity",
]
for path in files:
text = path.read_text()
print(f"--- {path} ---")
for symbol in symbols:
matches = [(i, line) for i, line in enumerate(text.splitlines(), 1)
if symbol in line]
print(f"{symbol}: {len(matches)} occurrence(s)")
for line_no, line in matches:
print(f" {line_no}: {line.strip()}")
markers = [(i, line) for i, line in enumerate(text.splitlines(), 1)
if re.match(r"^\s*(<<<<<<<|=======|>>>>>>>)(?:\s|$)", line)]
if markers:
print("merge conflict markers:")
for line_no, line in markers:
print(f" {line_no}: {line.strip()}")
PY
printf '%s\n' '--- restore.go surrounding the flag wiring ---'
sed -n '565,590p;870,895p' br/pkg/task/restore.goRepository: pingcap/tidb
Length of output: 3861
Resolve the merge conflicts in br/pkg/task/restore.go.
The restore flag is wired correctly. Remove the unresolved <<<<<<<, =======, and >>>>>>> markers at lines 279-287, 408-418, 564-582, and 828-887 because they prevent compilation.
🤖 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/snap_client/client.go` around lines 276 - 286, Resolve the
merge conflicts in restore.go by removing all unresolved conflict markers at the
specified sections and retaining the correct merged implementation for each
block. Preserve the existing restore-flag wiring through
SnapClient.SetCheckPrivilegeTableRowsCollateCompatiblity and
GetCheckPrivilegeTableRowsCollateCompatiblity, ensuring the file compiles.
| var createCallBacks []func(*SnapFileImporter) error | ||
| var closeCallBacks []func(*SnapFileImporter) error | ||
|
|
||
| createCallBacks = append(createCallBacks, func(importer *SnapFileImporter) error { | ||
| return setFn(importer, rc.rateLimit) | ||
| }) | ||
| closeCallBacks = append(createCallBacks, func(importer *SnapFileImporter) error { | ||
| return setFn(importer, 0) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
closeCallBacks is built from createCallBacks.
Line 71 appends the close callback to createCallBacks, not to closeCallBacks. The close list then also contains the create callback, so closing the importer sets the rate limit twice: first to rc.rateLimit, then to 0. The final value is correct, but the extra call makes the mock diverge from production behavior.
🐛 Proposed fix
- closeCallBacks = append(createCallBacks, func(importer *SnapFileImporter) error {
+ closeCallBacks = append(closeCallBacks, func(importer *SnapFileImporter) error {
return setFn(importer, 0)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var createCallBacks []func(*SnapFileImporter) error | |
| var closeCallBacks []func(*SnapFileImporter) error | |
| createCallBacks = append(createCallBacks, func(importer *SnapFileImporter) error { | |
| return setFn(importer, rc.rateLimit) | |
| }) | |
| closeCallBacks = append(createCallBacks, func(importer *SnapFileImporter) error { | |
| return setFn(importer, 0) | |
| }) | |
| var createCallBacks []func(*SnapFileImporter) error | |
| var closeCallBacks []func(*SnapFileImporter) error | |
| createCallBacks = append(createCallBacks, func(importer *SnapFileImporter) error { | |
| return setFn(importer, rc.rateLimit) | |
| }) | |
| closeCallBacks = append(closeCallBacks, func(importer *SnapFileImporter) error { | |
| return setFn(importer, 0) | |
| }) |
🤖 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/snap_client/export_test.go` around lines 65 - 73, Update the
callback registration in the test setup so the close callback is appended to
closeCallBacks rather than createCallBacks. Keep createCallBacks containing only
the rate-limit initialization callback, ensuring close execution invokes only
the callback that resets the rate limit.
| // skip check collate but type mismatch | ||
| mockedDBTI = dbTI.Clone() | ||
| mockedDBTI.Columns[1].SetCollate("utf8mb4_bin") | ||
| mockedUserTI.Columns[1].FieldType.SetFlen(2000) // Columns[1] is `DB` char(64) | ||
| _, err = snapclient.CheckSysTableCompatibility(cluster.Domain, []*metautil.Table{{ | ||
| DB: tmpSysDB, | ||
| Info: mockedDBTI, | ||
| }}, true) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The type-mismatch case mutates the wrong table info.
The comment states "skip check collate but type mismatch". The code changes mockedUserTI.Columns[1], but it passes mockedDBTI to CheckSysTableCompatibility. The mutation has no effect on the assertion, so this case duplicates the previous case.
🧪 Proposed fix
mockedDBTI = dbTI.Clone()
mockedDBTI.Columns[1].SetCollate("utf8mb4_bin")
- mockedUserTI.Columns[1].FieldType.SetFlen(2000) // Columns[1] is `DB` char(64)
+ mockedDBTI.Columns[1].FieldType.SetFlen(2000) // Columns[1] is `DB` char(64)
_, err = snapclient.CheckSysTableCompatibility(cluster.Domain, []*metautil.Table{{
DB: tmpSysDB,
Info: mockedDBTI,
}}, true)
- require.NoError(t, err)
+ require.True(t, berrors.ErrRestoreIncompatibleSys.Equal(err))Confirm the expected result before you apply the assertion change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // skip check collate but type mismatch | |
| mockedDBTI = dbTI.Clone() | |
| mockedDBTI.Columns[1].SetCollate("utf8mb4_bin") | |
| mockedUserTI.Columns[1].FieldType.SetFlen(2000) // Columns[1] is `DB` char(64) | |
| _, err = snapclient.CheckSysTableCompatibility(cluster.Domain, []*metautil.Table{{ | |
| DB: tmpSysDB, | |
| Info: mockedDBTI, | |
| }}, true) | |
| require.NoError(t, err) | |
| // skip check collate but type mismatch | |
| mockedDBTI = dbTI.Clone() | |
| mockedDBTI.Columns[1].SetCollate("utf8mb4_bin") | |
| mockedDBTI.Columns[1].FieldType.SetFlen(2000) // Columns[1] is `DB` char(64) | |
| _, err = snapclient.CheckSysTableCompatibility(cluster.Domain, []*metautil.Table{{ | |
| DB: tmpSysDB, | |
| Info: mockedDBTI, | |
| }}, true) | |
| require.True(t, berrors.ErrRestoreIncompatibleSys.Equal(err)) |
🤖 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/snap_client/systable_restore_test.go` around lines 137 - 145,
Update the type-mismatch setup in the test around CheckSysTableCompatibility to
mutate the same table info that is passed to it: change the relevant column on
mockedDBTI rather than mockedUserTI. Preserve the expected no-error assertion
for this case, confirming the collate difference remains skipped while the DB
column type mismatch is exercised.
| func TestMonitorTheSystemTableIncremental(t *testing.T) { | ||
| require.Equal(t, int64(253), session.CurrentBootstrapVersion) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Read the bootstrap version declared on this branch.
rg -n 'CurrentBootstrapVersion\s*=' pkg/sessionRepository: pingcap/tidb
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- branch and status ---'
git branch --show-current
git status --short
printf '%s\n' '--- bootstrap declarations and references ---'
rg -n -C 4 'currentBootstrapVersion|CurrentBootstrapVersion' pkg/session br/pkg/restore/snap_client/systable_restore_test.go
printf '%s\n' '--- test context ---'
sed -n '360,415p' br/pkg/restore/snap_client/systable_restore_test.go
printf '%s\n' '--- nearby session testutil ---'
sed -n '1,70p' pkg/session/testutil.goRepository: pingcap/tidb
Length of output: 50368
Update the bootstrap-version assertion.
release-8.1 defines session.CurrentBootstrapVersion as version199, so the assertion for 253 fails. Use the branch value.
🤖 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/snap_client/systable_restore_test.go` around lines 395 - 397,
Update TestMonitorTheSystemTableIncremental to assert
session.CurrentBootstrapVersion equals the release-8.1 branch value version199
instead of 253.
| if colCount != len(collateCompatibilityColumnMap.columns) { | ||
| return errors.Annotatef(berrors.ErrRestoreIncompatibleSys, | ||
| "incompatible column collate, downstream table %s.%s has only %d columns with collate utf8mb4_bin", | ||
| dbNameL, tableNameL, colCount) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the collate name in the downstream error message.
This branch validates the downstream collate utf8mb4_general_ci, but the message reports utf8mb4_bin. The message misleads during diagnosis.
🔤 Proposed fix
if colCount != len(collateCompatibilityColumnMap.columns) {
return errors.Annotatef(berrors.ErrRestoreIncompatibleSys,
- "incompatible column collate, downstream table %s.%s has only %d columns with collate utf8mb4_bin",
+ "incompatible column collate, downstream table %s.%s has only %d columns with collate utf8mb4_general_ci",
dbNameL, tableNameL, colCount)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if colCount != len(collateCompatibilityColumnMap.columns) { | |
| return errors.Annotatef(berrors.ErrRestoreIncompatibleSys, | |
| "incompatible column collate, downstream table %s.%s has only %d columns with collate utf8mb4_bin", | |
| dbNameL, tableNameL, colCount) | |
| } | |
| if colCount != len(collateCompatibilityColumnMap.columns) { | |
| return errors.Annotatef(berrors.ErrRestoreIncompatibleSys, | |
| "incompatible column collate, downstream table %s.%s has only %d columns with collate utf8mb4_general_ci", | |
| dbNameL, tableNameL, colCount) | |
| } |
🤖 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/snap_client/systable_restore.go` around lines 807 - 811,
Update the error message in the colCount validation branch to report
utf8mb4_general_ci instead of utf8mb4_bin, keeping the existing error type,
formatting, and control flow unchanged.
| <<<<<<< HEAD | ||
| ======= | ||
| flags.Bool(flagAllowPITRFromIncremental, true, "whether make incremental restore compatible with later log restore"+ | ||
| " default is true, the incremental restore will not perform rewrite on the incremental data"+ | ||
| " meanwhile the incremental restore will not allow to restore 3 backfilled type ddl jobs,"+ | ||
| " these ddl jobs are Add index, Modify column and Reorganize partition") | ||
| flags.Bool(FlagSysCheckCollation, false, "whether check the privileges table rows to permit to restore the privilege data"+ | ||
| " from utf8mb4_bin collate column to utf8mb4_general_ci collate column") | ||
| >>>>>>> 0cb39391dce (br: support restore privileges tables from v6.5 to v7.5 (#64668)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved merge conflict markers block compilation.
The file still contains <<<<<<< HEAD, =======, and >>>>>>> 0cb39391dce markers at lines 279-287, 408-418, 564-582, 828, and 883-887. Go cannot parse this file. Resolve every conflict before merge.
Two resolutions need attention:
- Lines 564-582: the
HEADside callsrestore.CheckKeyspaceBREnable,client.LoadRestoreStores(ctx), andclient.SetConcurrency(uint(cfg.Concurrency)). The incoming side calls onlyclient.SetRewriteMode(ctx)andclient.SetCheckPrivilegeTableRowsCollateCompatiblity(cfg.SysCheckCollation). If you keep only the incoming side, the restore stores never load and the concurrency stays unset. KeepLoadRestoreStoresandSetConcurrencytogether with the two new calls, and confirm thatSnapClient.SetRewriteMode(ctx)exists on this branch. - Lines 883-893: the
if err != nilbody and theloadSysTablePhysicalfallback sit outside the conflict block. After you resolve the markers, verify thatcanLoadSysTablePhysicalanderrare declared in the retained branch, and that theHEADcode the marker at line 828 hides is preserved or intentionally dropped.
Also applies to: 408-418, 564-582, 828-828, 883-893
🤖 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/task/restore.go` around lines 279 - 287, Resolve every merge conflict
marker in br/pkg/task/restore.go, including the blocks around flag registration,
restore client setup, and physical system-table loading. In the restore setup,
retain restore.CheckKeyspaceBREnable, client.LoadRestoreStores(ctx), and
client.SetConcurrency(uint(cfg.Concurrency)) together with SetRewriteMode(ctx)
and SetCheckPrivilegeTableRowsCollateCompatiblity(cfg.SysCheckCollation),
confirming SnapClient.SetRewriteMode exists. Ensure the retained branch declares
canLoadSysTablePhysical and err before the subsequent error handling and
fallback, and preserve or intentionally remove the code hidden near line 828.
| typeEq, collateEq := IsTypeCompatible(*src, *target) | ||
| require.False(t, typeEq) | ||
| require.True(t, collateEq) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This case does not exercise the length check.
The block comment states "src flen > target", but the target sets a flag value, not a length. The value 99 sets NotNullFlag, PriKeyFlag, UnsignedFlag, and ZerofillFlag. IsTypeCompatible then returns false at the not-null flag check, before it compares lengths. The assertion passes for the wrong reason.
Set the target length so the case reaches the length comparison.
🤖 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/utils/misc_test.go` around lines 81 - 83, Update the test case around
IsTypeCompatible so the target column’s length is explicitly smaller than the
source length, rather than assigning 99 to its flag field. Preserve the existing
assertions and ensure the inputs reach the length-comparison branch instead of
returning early on flag incompatibility.
| func IsTypeCompatible(src types.FieldType, target types.FieldType) (typeEq, collateEq bool) { | ||
| collateEq = src.GetCollate() == target.GetCollate() | ||
| if mysql.HasNotNullFlag(src.GetFlag()) != mysql.HasNotNullFlag(target.GetFlag()) { | ||
| return false | ||
| return false, collateEq | ||
| } | ||
| if mysql.HasUnsignedFlag(src.GetFlag()) != mysql.HasUnsignedFlag(target.GetFlag()) { | ||
| return false | ||
| return false, collateEq | ||
| } | ||
| srcEType, dstEType := src.EvalType(), target.EvalType() | ||
| if srcEType != dstEType { | ||
| return false | ||
| return false, collateEq |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all IsTypeCompatible call sites and check the assignment arity.
rg -nP --type=go -C3 '\bIsTypeCompatible\s*\('Repository: pingcap/tidb
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' 'Tracked Go files containing IsTypeCompatible:'
git grep -n -w -- 'IsTypeCompatible' -- '*.go' || true
printf '%s\n' 'Definition and client.go context:'
git grep -n -C 5 -- 'func IsTypeCompatible' -- 'br/pkg/utils/misc.go' || true
git grep -n -C 5 -- 'IsTypeCompatible' -- 'br/pkg/restore/client.go' || true
printf '%s\n' 'Relevant tracked files:'
git ls-files 'br/pkg/utils/misc.go' 'br/pkg/restore/client.go'Repository: pingcap/tidb
Length of output: 3976
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' 'Restore client call context:'
sed -n '1228,1270p' br/pkg/restore/client.go
printf '%s\n' 'Other caller context:'
sed -n '690,718p' br/pkg/restore/snap_client/systable_restore.go
printf '%s\n' 'Function return paths:'
sed -n '53,100p' br/pkg/utils/misc.goRepository: pingcap/tidb
Length of output: 5556
Update the br/pkg/restore/client.go:1256 call for the two-value return. IsTypeCompatible returns (typeEq, collateEq), but this call uses it as one boolean and does not compile.
🤖 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/utils/misc.go` around lines 60 - 70, Update the IsTypeCompatible call
in the restore client type-checking logic to receive both returned values, using
the typeEq result for the existing compatibility decision and handling collateEq
as required by the surrounding logic. Ensure the call no longer treats the
two-value return as a single boolean and compiles successfully.
This branch has not been deployed
2 participants
This is an automated cherry-pick of #64668
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
New Features
Bug Fixes
Configuration
sys-check-collationrestore option.