session: initialize masking policy table before upgrade DDL#69763
session: initialize masking policy table before upgrade DDL#69763expxiaoli wants to merge 7 commits into
Conversation
ti-chi-bot
Bot
added
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBootstrap now creates missing masking-policy system tables during eligible upgrades, while masking-policy DDL paths remove bootstrap-specific suppression of missing-table errors. Upgrade and restart tests verify ID allocation, table recreation, and rename behavior. ChangesMasking policy bootstrap
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant bootstrapSessionImpl
participant initBootstrapDependentTables
participant createAndSplitTablesIfNotExists
participant BootstrapUpgradeTests
bootstrapSessionImpl->>initBootstrapDependentTables: initialize dependent tables
initBootstrapDependentTables->>createAndSplitTablesIfNotExists: create missing masking-policy tables
createAndSplitTablesIfNotExists-->>bootstrapSessionImpl: return initialization status
BootstrapUpgradeTests->>BootstrapUpgradeTests: verify IDs, recreation, and restart behavior
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = initBootstrapDependentTables(store) |
There was a problem hiding this comment.
maybe put into InitDDLTables, and consider the difference between classic and nextgen
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69763 +/- ##
================================================
- Coverage 76.3188% 75.5806% -0.7383%
================================================
Files 2041 2084 +43
Lines 560066 582304 +22238
================================================
+ Hits 427436 440109 +12673
- Misses 131729 140082 +8353
- Partials 901 2113 +1212
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/retest-required |
|
Looks good to me. The change is minimal and targeted, which I appreciate. LGTM once CI is green. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ddl/masking_policy_internal_test.go (1)
26-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding test cases for
updateMaskingPolicyTableIDAfterTruncateandupdateMaskingPolicyNamesAfterRename.Both functions had error handling changes and use the same failpoint path, but are not covered.
updateMaskingPolicyNamesAfterRenameis especially worth testing since its signature changed toctx context.Context. Adding entries to the existing test table is low-effort:♻️ Suggested additional test cases
tests := []struct { name string run func() error }{ { name: "table", run: func() error { return w.dropMaskingPoliciesOnTable(jobCtx, 1) }, }, { name: "column", run: func() error { return w.dropMaskingPoliciesOnColumn(jobCtx, 1, 1) }, }, + { + name: "truncate", + run: func() error { + return w.updateMaskingPolicyTableIDAfterTruncate(jobCtx, 1, 2) + }, + }, + { + name: "rename", + run: func() error { + return w.updateMaskingPolicyNamesAfterRename(context.Background(), 1, + ast.CIStr{}, ast.CIStr{}, ast.CIStr{}, ast.CIStr{}) + }, + }, }🤖 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 `@pkg/ddl/masking_policy_internal_test.go` around lines 26 - 56, Extend TestDropMaskingPoliciesRequiresSysTable with table entries invoking updateMaskingPolicyTableIDAfterTruncate and updateMaskingPolicyNamesAfterRename, passing the existing job context as required by the updated context-aware signature. Assert both return an error matching infoschema.ErrTableNotExists, consistent with the existing table and column cases.
🤖 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 `@pkg/ddl/masking_policy_internal_test.go`:
- Around line 26-56: Extend TestDropMaskingPoliciesRequiresSysTable with table
entries invoking updateMaskingPolicyTableIDAfterTruncate and
updateMaskingPolicyNamesAfterRename, passing the existing job context as
required by the updated context-aware signature. Assert both return an error
matching infoschema.ErrTableNotExists, consistent with the existing table and
column cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 879dda0f-73ea-4012-86df-33f1dc15bac8
📒 Files selected for processing (6)
.agents/skills/tidb-test-guidelines/references/ddl-case-map.md.agents/skills/tidb-test-guidelines/references/session-case-map.mdpkg/ddl/masking_policy.gopkg/ddl/masking_policy_internal_test.gopkg/session/session.gopkg/session/test/bootstraptest/bootstrap_upgrade_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/session/session.go
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/retest-required |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ddl/masking_policy_internal_test.go (1)
32-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRestore missing-table coverage for modified-column synchronization.
syncMaskingPolicyForModifiedColumnnow propagates the lookup error (pkg/ddl/masking_policy.go, Lines 657-660), but this table no longer exercises that changed path. Add a non-nil table/old-column/new-column case assertinginfoschema.ErrTableNotExists; the replaced test covered it.🤖 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 `@pkg/ddl/masking_policy_internal_test.go` around lines 32 - 65, Restore a test case in the masking-policy test table that invokes syncMaskingPolicyForModifiedColumn with non-nil table, old-column, and new-column inputs. Assert that the call returns infoschema.ErrTableNotExists, preserving coverage for the propagated missing-table lookup error alongside the existing table, column, truncate, and rename cases.
🤖 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 `@pkg/ddl/masking_policy_internal_test.go`:
- Around line 32-65: Restore a test case in the masking-policy test table that
invokes syncMaskingPolicyForModifiedColumn with non-nil table, old-column, and
new-column inputs. Assert that the call returns infoschema.ErrTableNotExists,
preserving coverage for the propagated missing-table lookup error alongside the
existing table, column, truncate, and rename cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3ac040e-d170-41d4-8c12-288f4d1295a0
📒 Files selected for processing (8)
.agents/skills/tidb-test-guidelines/references/ddl-case-map.md.agents/skills/tidb-test-guidelines/references/session-case-map.mdpkg/ddl/ddl.gopkg/ddl/masking_policy.gopkg/ddl/masking_policy_internal_test.gopkg/ddl/table.gopkg/session/session.gopkg/session/test/bootstraptest/bootstrap_upgrade_test.go
💤 Files with no reviewable changes (1)
- pkg/ddl/ddl.go
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/ddl/masking_policy.go (1)
537-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the new "table must already exist" invariant.
These functions used to swallow
ErrTableNotExistsduring bootstrap/upgrade; now they always propagate it, relying oninitBootstrapDependentTables(pkg/session/session.go) to have createdmysql.tidb_masking_policybefore any upgrade DDL runs. That cross-file contract isn't obvious from this file alone — a short comment (similar to the one added ondropMaskingPoliciesByDBName) would help future maintainers avoid reintroducing the original bug (issue#69464) if this invariant is ever broken.As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs."
📝 Example comment to add near dropMaskingPoliciesOnTable
+// dropMaskingPoliciesOnTable assumes mysql.tidb_masking_policy already exists; +// bootstrap guarantees this via initBootstrapDependentTables before any +// upgrade DDL that may reach this code path runs (see issue `#69464`). func (w *worker) dropMaskingPoliciesOnTable(jobCtx *jobContext, tableID int64) error {Also applies to: 589-600, 569-587, 604-629, 647-692
🤖 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 `@pkg/ddl/masking_policy.go` around lines 537 - 548, Add a concise comment near dropMaskingPoliciesOnTable and the related masking-policy cleanup functions explaining that mysql.tidb_masking_policy must already exist because initBootstrapDependentTables creates it before upgrade DDL, so ErrTableNotExists must be propagated rather than swallowed. Keep the comment focused on this cross-file invariant and apply it consistently to all listed cleanup helpers.Source: Coding guidelines
pkg/ddl/masking_policy_internal_test.go (1)
28-86: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo test coverage for
dropMaskingPoliciesByDBName's missing-table behavior.This is the only masking-policy DDL helper that still swallows
ErrTableNotExists(best-effort for DROP DATABASE). Since it callsw.sess.Executedirectly instead of going throughqueryMaskingPoliciesFromSysTable, it can't be dropped into this failpoint-based table (a bare&worker{}would nil-panic onw.sess). Given it's the one function with the opposite contract from everything else touched in this PR, it seems worth a dedicated regression test (e.g., via the existing session/testkit-based masking policy test setup) asserting it returnsnilwhen the sys table is missing.🤖 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 `@pkg/ddl/masking_policy_internal_test.go` around lines 28 - 86, Add dedicated regression coverage for dropMaskingPoliciesByDBName using the existing session/testkit masking-policy setup, because the current bare worker test cannot exercise its direct w.sess.Execute path. Simulate a missing masking-policy system table and assert dropMaskingPoliciesByDBName returns nil, preserving its best-effort DROP DATABASE contract rather than expecting ErrTableNotExists.
🤖 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 `@pkg/ddl/masking_policy_internal_test.go`:
- Around line 28-86: Add dedicated regression coverage for
dropMaskingPoliciesByDBName using the existing session/testkit masking-policy
setup, because the current bare worker test cannot exercise its direct
w.sess.Execute path. Simulate a missing masking-policy system table and assert
dropMaskingPoliciesByDBName returns nil, preserving its best-effort DROP
DATABASE contract rather than expecting ErrTableNotExists.
In `@pkg/ddl/masking_policy.go`:
- Around line 537-548: Add a concise comment near dropMaskingPoliciesOnTable and
the related masking-policy cleanup functions explaining that
mysql.tidb_masking_policy must already exist because
initBootstrapDependentTables creates it before upgrade DDL, so ErrTableNotExists
must be propagated rather than swallowed. Keep the comment focused on this
cross-file invariant and apply it consistently to all listed cleanup helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1234abcf-1e50-441a-a764-7018424ea93d
📒 Files selected for processing (8)
.agents/skills/tidb-test-guidelines/references/ddl-case-map.md.agents/skills/tidb-test-guidelines/references/session-case-map.mdpkg/ddl/ddl.gopkg/ddl/masking_policy.gopkg/ddl/masking_policy_internal_test.gopkg/ddl/table.gopkg/session/session.gopkg/session/test/bootstraptest/bootstrap_upgrade_test.go
💤 Files with no reviewable changes (1)
- pkg/ddl/ddl.go
Thanks for the suggestions. We prefer not to change the code in this PR. |
|
/retest-required |
| // bootstrap quickly, after bootstrapped, we will reset the lease time. | ||
| // TODO: Using a bootstrap tool for doing this may be better later. | ||
| func runInBootstrapSession(store kv.Storage, ver int64) { | ||
| func runInBootstrapSession(store kv.Storage, ver int64) error { |
There was a problem hiding this comment.
currently, this method use log.Fatal to exit directly on non-retryable error, not return it
There was a problem hiding this comment.
I prefer to keep this design.
At this point we already hold the upgrade lock, and returning lets defer releaseFn() unlock and close the etcd session. log.Fatal would skip defers in production and leave the 10-second lease to expire.
Additionally, BootstrapSession already has an error-return contract and its caller handles startup failures, so propagating the error is safer and more testable.
| // TODO remove this after we can refactor below code out in this case. | ||
| logutil.BgLogger().Info("[upgrade] already upgraded by other nodes, switch to normal mode") | ||
| startMode = ddl.Normal | ||
| } else if err := initBootstrapDependentTables(store, currVer); err != nil { |
There was a problem hiding this comment.
please add some explain comment on why it have to be run under the upgrade lock
There was a problem hiding this comment.
Agree, I will add comment later.
| dbID int64, | ||
| tables []TableBasicInfo, | ||
| ) error { | ||
| existingTables, err := t.ListTables(ctx, dbID) |
There was a problem hiding this comment.
please comment on why we have to check the table existance first, while InitDDLTables don't need
There was a problem hiding this comment.
Agree, I will add comment later.
| // initBootstrapDependentTables creates system tables that classic kernel upgrade | ||
| // DDL may consult before the ordinary upgrade DDL reaches their creation step. | ||
| func initBootstrapDependentTables(store kv.Storage, ver int64) error { | ||
| if !kerneltype.IsClassic() || ver <= notBootstrapped || ver >= version260 || currentBootstrapVersion < version260 { |
There was a problem hiding this comment.
please comment, why those case we can skip
There was a problem hiding this comment.
Agree, I will add comment later.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter 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 |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
ti-chi-bot
Bot
added
size/XL
|
@expxiaoli: 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. |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
3 participants
What problem does this PR solve?
Issue Number: close #69464
Problem Summary:
During rolling upgrade, a newly started TiDB may repeatedly crash in bootstrap upgrade.
upgradeToVer190can run reentrantMODIFY COLUMNDDLs beforeupgradeToVer260createsmysql.tidb_masking_policy. The masking policy sync path used by ordinary DDL may querymysql.tidb_masking_policy, so the DDL job fails withTable 'mysql.tidb_masking_policy' doesn't exist, and bootstrap upgrade exits fatally.What changed and how does it work?
This PR reverts the previous DDL-layer workaround that tolerated a missing masking policy system table during bootstrap or upgrade.
Instead, it initializes bootstrap-dependent system tables before running bootstrap or upgrade DDL. Specifically,
mysql.tidb_masking_policyis created through Meta KV beforerunInBootstrapSession, so upgrade DDLs that consult masking policy metadata can run safely even when the stored bootstrap version is older thanversion260.The existing
upgradeToVer260CREATE TABLE IF NOT EXISTSstep is kept as an idempotent versioned upgrade step.This PR also extends the existing masking policy bootstrap upgrade test to cover upgrading from bootstrap version
189, which reproduces the failingupgradeToVer190path. The existing254coverage is kept.Check List
Tests
Executed:
GOPATH=/Users/xiaoli/go GOSUMDB=sum.golang.org ./tools/check/failpoint-go-test.sh pkg/session/test/bootstraptest -run TestUpgradeVersion260MaskingPolicy -count=1GOPATH=/Users/xiaoli/go GOSUMDB=sum.golang.org make lintgit diff --checkSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
mysql.tidb_masking_policyis missing, consistently returning “table not exists” without special bootstrap/upgrade suppression.