ddl: Handle mlog name confliction for materialized view#68841
Open
xzhangxian1008 wants to merge 8 commits into
Open
ddl: Handle mlog name confliction for materialized view#68841xzhangxian1008 wants to merge 8 commits into
xzhangxian1008 wants to merge 8 commits into
Conversation
ti-chi-bot
Bot
added
the
release-note
|
[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 |
ti-chi-bot
Bot
added
the
size/XXL
|
Hi @xzhangxian1008. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughCentralizes MLog name generation and lookup: adds exported generation/lookup primitives, exposes GenerateMLogTableName on executors, changes CreateMaterializedViewLog to accept a generated name, implements retry-on-conflict in DDL executor, updates MV DDL paths to use resolved MLog metadata, and adds tests and small build tweaks. ChangesMaterialized View Log Name Generation and Conflict Handling
Sequence Diagram(s)sequenceDiagram
participant Client as DDL Client
participant DDLExec
participant Executor
participant Naming
Client->>DDLExec: Next(CREATE MATERIALIZED VIEW LOG)
loop Retry on conflict
DDLExec->>Executor: GenerateMLogTableName(ctx, stmt)
Executor->>Naming: compute candidate name
Naming-->>Executor: mlogTableName
DDLExec->>Executor: CreateMaterializedViewLog(ctx, stmt, mlogTableName)
alt Name conflict
Executor-->>DDLExec: conflict error
DDLExec->>DDLExec: wait and retry
else Success
Executor-->>DDLExec: nil
DDLExec-->>Client: OK
else Other error
Executor-->>DDLExec: error
DDLExec-->>Client: error
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/ddl/schematracker/checker.go (1)
279-282: ⚡ Quick winCompare tracker-generated and real-generated MLog names here.
Right now the checker delegates name generation only to
realExecutor, then feeds that same name intod.tracker.CreateMaterializedViewLog. That means a bug inSchemaTracker.GenerateMLogTableNamewould no longer be caught by this checker, even though name-conflict handling is the main behavior being changed in this PR.Suggested change
func (d *Checker) GenerateMLogTableName(ctx sessionctx.Context, s *ast.CreateMaterializedViewLogStmt) (string, error) { - return d.realExecutor.GenerateMLogTableName(ctx, s) + name, err := d.realExecutor.GenerateMLogTableName(ctx, s) + if err != nil || d.closed.Load() { + return name, err + } + trackerName, err := d.tracker.GenerateMLogTableName(ctx, s) + if err != nil { + panic(err) + } + if name != trackerName { + panic(fmt.Sprintf("inconsistent mlog name, real ddl: %s, schematracker: %s", name, trackerName)) + } + return name, nil }🤖 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/schematracker/checker.go` around lines 279 - 282, The current Checker.GenerateMLogTableName just delegates to d.realExecutor.GenerateMLogTableName and then passes that same name to d.tracker.CreateMaterializedViewLog, so mismatches in SchemaTracker.GenerateMLogTableName are never detected; change GenerateMLogTableName to call both d.realExecutor.GenerateMLogTableName(ctx, s) and d.tracker.GenerateMLogTableName(ctx, s), compare the two returned names, and if they differ return an error (or wrap the discrepancy in the returned error) so the checker fails fast; keep the successful behavior unchanged by continuing to use the realExecutor name for downstream calls like d.tracker.CreateMaterializedViewLog when names match.
🤖 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 `@pkg/ddl/executor.go`:
- Around line 1122-1123: This path returns infoschema.ErrTableExists for the
case where baseTable.Meta().MaterializedViewBase != nil and baseInfo.MLogID !=
0, which the caller treats as retryable; change it to return a distinct,
non-retryable error so callers won't retry on an already-created MLog. Replace
the infoschema.ErrTableExists return in that branch with a new clearly-named
error (e.g., infoschema.ErrMLogAlreadyExists or a sentinel error variable) or
wrap the message using a non-retryable error constructor, ensuring the condition
references baseInfo, baseTable.Meta().MaterializedViewBase and MLogID so the
returned error is unique from table-name-collision ErrTableExists.
In `@pkg/ddl/materialized_view.go`:
- Around line 104-124: The length checks are using candidate.L (lowercased
string) which can expand in runes for some Unicode characters; update both
occurrences to use utf8.RuneCountInString(candidate.O) instead of candidate.L in
the materialized view log name logic (the block that calls checkTableExistence,
the conditional returning candidate.O, and the loop that generates names via
nextMLogTableNameNumber/MLogShortTableNameSeq/materializedViewLogTablePrefix)
and keep the dbterror.ErrTooLongIdent handling unchanged.
In `@pkg/ddl/schematracker/dm_tracker.go`:
- Around line 234-237: GenerateMLogTableName currently returns a hardcoded "not
implemented" which causes CREATE MATERIALIZED VIEW LOG to fail; implement it in
SchemaTracker to compute and return the correct materialized-view-log table name
from the provided ast.CreateMaterializedViewLogStmt and sessionctx.Context
(validate the stmt, derive schema/name, apply the project's naming convention
and uniqueness rules, and return an error only for validation failures). Locate
the method named GenerateMLogTableName on type SchemaTracker and replace the
stub with logic that extracts the target database/table from the ast node,
constructs the mlog table name (consistent with existing naming helpers used
elsewhere in SchemaTracker), and returns that name and any validation error
instead of the current hardcoded error.
In `@pkg/executor/ddl.go`:
- Around line 340-344: The current retry loop treats any
infoschema.ErrTableExists returned from CreateMaterializedViewLog as a
name-conflict and keeps retrying, but CreateMaterializedViewLog also returns
ErrTableExists for terminal cases like "base table already has an MLog"; change
the contract so GenerateMLogTableName/CreateMaterializedViewLog return a
distinct sentinel error (e.g., ErrMlogNameConflict) for pure table-name races
and keep the existing ErrTableExists for terminal MLog-already-exists
conditions, then update the retry logic in the loop that calls
e.ddlExecutor.CreateMaterializedViewLog(e.Ctx(), stmt, mlogTableName) to only
continue/retry when the error equals the new ErrMlogNameConflict and otherwise
surface/return the error. Ensure the new sentinel is documented and used by
GenerateMLogTableName and CreateMaterializedViewLog consistently.
---
Nitpick comments:
In `@pkg/ddl/schematracker/checker.go`:
- Around line 279-282: The current Checker.GenerateMLogTableName just delegates
to d.realExecutor.GenerateMLogTableName and then passes that same name to
d.tracker.CreateMaterializedViewLog, so mismatches in
SchemaTracker.GenerateMLogTableName are never detected; change
GenerateMLogTableName to call both d.realExecutor.GenerateMLogTableName(ctx, s)
and d.tracker.GenerateMLogTableName(ctx, s), compare the two returned names, and
if they differ return an error (or wrap the discrepancy in the returned error)
so the checker fails fast; keep the successful behavior unchanged by continuing
to use the realExecutor name for downstream calls like
d.tracker.CreateMaterializedViewLog when names match.
🪄 Autofix (Beta)
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
Run ID: ec03e328-3f91-41f3-a33b-7cb1c2be7bb6
📒 Files selected for processing (12)
pkg/ddl/ddl_test.gopkg/ddl/executor.gopkg/ddl/materialized_view.gopkg/ddl/schematracker/BUILD.bazelpkg/ddl/schematracker/checker.gopkg/ddl/schematracker/dm_tracker.gopkg/ddl/schematracker/dm_tracker_test.gopkg/executor/BUILD.bazelpkg/executor/ddl.gopkg/executor/ddl_test.gopkg/executor/materialized_view.gopkg/executor/test/ddl/mview_log_ddl_test.go
Comment on lines
+104
to
+124
| exists, err := checkTableExistence(candidate) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| if !exists && utf8.RuneCountInString(candidate.L) <= mysql.MaxTableNameLength { | ||
| return candidate.O, nil | ||
| } | ||
|
|
||
| for { | ||
| var err error | ||
| number, err := nextMLogTableNameNumber( | ||
| &MLogShortTableNameSeq, | ||
| "materialized view log short table name number is out of range", | ||
| ) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| candidate = pmodel.NewCIStr(materializedViewLogTablePrefix + strconv.FormatUint(number, 10)) | ||
| if utf8.RuneCountInString(candidate.L) > mysql.MaxTableNameLength { | ||
| return "", dbterror.ErrTooLongIdent.GenWithStackByArgs(candidate) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'type CIStr|func NewCIStr|RuneCountInString\(candidate\.[LO]\)' pkg/parser pkg/ddl/materialized_view.go
python - <<'PY'
samples = ["İ", "I", "ß"]
for s in samples:
print(repr(s), "orig_runes=", len(s), "lower=", repr(s.lower()), "lower_runes=", len(s.lower()))
PYRepository: pingcap/tidb
Length of output: 1434
Fix identifier length validation to use candidate.O (not candidate.L)
CIStr.L is produced by strings.ToLower, and Unicode lowercasing can increase rune count (e.g. İ -> i̇). Using utf8.RuneCountInString(candidate.L) at Lines 109 and 123 can therefore incorrectly trigger the short-name fallback / dbterror.ErrTooLongIdent for valid identifiers. Use candidate.O for the length check.
Suggested fix
- if !exists && utf8.RuneCountInString(candidate.L) <= mysql.MaxTableNameLength {
+ if !exists && utf8.RuneCountInString(candidate.O) <= mysql.MaxTableNameLength {
return candidate.O, nil
}
@@
- if utf8.RuneCountInString(candidate.L) > mysql.MaxTableNameLength {
+ if utf8.RuneCountInString(candidate.O) > mysql.MaxTableNameLength {
return "", dbterror.ErrTooLongIdent.GenWithStackByArgs(candidate)
}📝 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.
Suggested change
| exists, err := checkTableExistence(candidate) | |
| if err != nil { | |
| return "", err | |
| } | |
| if !exists && utf8.RuneCountInString(candidate.L) <= mysql.MaxTableNameLength { | |
| return candidate.O, nil | |
| } | |
| for { | |
| var err error | |
| number, err := nextMLogTableNameNumber( | |
| &MLogShortTableNameSeq, | |
| "materialized view log short table name number is out of range", | |
| ) | |
| if err != nil { | |
| return "", err | |
| } | |
| candidate = pmodel.NewCIStr(materializedViewLogTablePrefix + strconv.FormatUint(number, 10)) | |
| if utf8.RuneCountInString(candidate.L) > mysql.MaxTableNameLength { | |
| return "", dbterror.ErrTooLongIdent.GenWithStackByArgs(candidate) | |
| exists, err := checkTableExistence(candidate) | |
| if err != nil { | |
| return "", err | |
| } | |
| if !exists && utf8.RuneCountInString(candidate.O) <= mysql.MaxTableNameLength { | |
| return candidate.O, nil | |
| } | |
| for { | |
| var err error | |
| number, err := nextMLogTableNameNumber( | |
| &MLogShortTableNameSeq, | |
| "materialized view log short table name number is out of range", | |
| ) | |
| if err != nil { | |
| return "", err | |
| } | |
| candidate = pmodel.NewCIStr(materializedViewLogTablePrefix + strconv.FormatUint(number, 10)) | |
| if utf8.RuneCountInString(candidate.O) > mysql.MaxTableNameLength { | |
| return "", dbterror.ErrTooLongIdent.GenWithStackByArgs(candidate) |
🤖 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/materialized_view.go` around lines 104 - 124, The length checks are
using candidate.L (lowercased string) which can expand in runes for some Unicode
characters; update both occurrences to use utf8.RuneCountInString(candidate.O)
instead of candidate.L in the materialized view log name logic (the block that
calls checkTableExistence, the conditional returning candidate.O, and the loop
that generates names via
nextMLogTableNameNumber/MLogShortTableNameSeq/materializedViewLogTablePrefix)
and keep the dbterror.ErrTooLongIdent handling unchanged.
Comment on lines
+234
to
+237
| // GenerateMLogTableName implements the DDL interface. | ||
| func (*SchemaTracker) GenerateMLogTableName(sessionctx.Context, *ast.CreateMaterializedViewLogStmt) (string, error) { | ||
| return "", errors.New("not implemented") | ||
| } |
There was a problem hiding this comment.
Implement GenerateMLogTableName before wiring it into the executor path.
The new DDL flow now calls this method first, so returning "not implemented" makes every schema-tracker CREATE MATERIALIZED VIEW LOG fail at runtime.
Proposed direction
-// GenerateMLogTableName implements the DDL interface.
-func (*SchemaTracker) GenerateMLogTableName(sessionctx.Context, *ast.CreateMaterializedViewLogStmt) (string, error) {
- return "", errors.New("not implemented")
-}
+// GenerateMLogTableName implements the DDL interface.
+func (d *SchemaTracker) GenerateMLogTableName(ctx sessionctx.Context, s *ast.CreateMaterializedViewLogStmt) (string, error) {
+ // Mirror the same schema/base-table validation used by CreateMaterializedViewLog,
+ // then delegate the actual conflict-free name generation to ddl.GenerateMLogTableName
+ // with ddl.GetExistenceOfMLogTableNameChecker(..., InfoStoreAdaptor{inner: d.InfoStore}, ...).
+}🤖 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/schematracker/dm_tracker.go` around lines 234 - 237,
GenerateMLogTableName currently returns a hardcoded "not implemented" which
causes CREATE MATERIALIZED VIEW LOG to fail; implement it in SchemaTracker to
compute and return the correct materialized-view-log table name from the
provided ast.CreateMaterializedViewLogStmt and sessionctx.Context (validate the
stmt, derive schema/name, apply the project's naming convention and uniqueness
rules, and return an error only for validation failures). Locate the method
named GenerateMLogTableName on type SchemaTracker and replace the stub with
logic that extracts the target database/table from the ast node, constructs the
mlog table name (consistent with existing naming helpers used elsewhere in
SchemaTracker), and returns that name and any validation error instead of the
current hardcoded error.
Codecov Report❌ Patch coverage is Please upload reports for the commit 717fe24 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## feature/release-8.5-materialized-view-2603 #68841 +/- ##
===============================================================================
Coverage ? 22.2740%
===============================================================================
Files ? 1712
Lines ? 645266
Branches ? 0
===============================================================================
Hits ? 143727
Misses ? 483490
Partials ? 18049
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@xzhangxian1008: 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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/ddl/schematracker/dm_tracker.go (1)
252-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the new MLog error contract here.
This existence check now runs before the base-table guard and returns
infoschema.ErrTableExists, which collapses two distinct cases: retryable generated-name conflicts and “base table already has an MLog”. For the duplicate-create case, this masksddl.ErrMLogAlreadyExists; for genuine name clashes, callers lose theddl.ErrMLogTableNameConflictsentinel thatpkg/ddl/create_table.gonow emits.Suggested fix
mlogNameCIStr := pmodel.NewCIStr(mlogTableName) - mlogExists, err := ddl.GetExistenceOfMLogTableNameChecker( - context.Background(), - InfoStoreAdaptor{inner: d.InfoStore}, - schemaName, - )(mlogNameCIStr) - - if err != nil { - return err - } - - if mlogExists { - return infoschema.ErrTableExists.GenWithStackByArgs(ast.Ident{Schema: schemaName, Name: mlogNameCIStr}) - } - baseTable, err := d.TableClonedByName(schemaName, s.Table.Name) if err != nil { return err } if baseTable.IsView() || baseTable.IsSequence() || baseTable.TempTableType != model.TempTableNone { return dbterror.ErrWrongObject.GenWithStackByArgs(schemaName, s.Table.Name, "BASE TABLE") } if baseTable.MaterializedViewBase != nil && baseTable.MaterializedViewBase.MLogID != 0 { return ddl.ErrMLogAlreadyExists.GenWithStackByArgs(ast.Ident{Schema: schemaName, Name: baseTable.Name}) } + mlogExists, err := ddl.GetExistenceOfMLogTableNameChecker( + context.Background(), + InfoStoreAdaptor{inner: d.InfoStore}, + schemaName, + )(mlogNameCIStr) + if err != nil { + return err + } + if mlogExists { + return ddl.ErrMLogTableNameConflict.GenWithStackByArgs(ast.Ident{Schema: schemaName, Name: mlogNameCIStr}) + }🤖 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/schematracker/dm_tracker.go` around lines 252 - 275, The existence check for mlog currently runs before validating the base table, which masks the specific ddl errors; move the call to d.TableClonedByName and the base-table guards (checks using baseTable.IsView(), IsSequence(), TempTableType and baseTable.MaterializedViewBase.MLogID) to run before invoking ddl.GetExistenceOfMLogTableNameChecker/mlogExists so you can return ddl.ErrMLogAlreadyExists when the base table already has an MLog; afterwards run the mlog existence check and, on name conflict, return the specific ddl.ErrMLogTableNameConflict (using the same ast.Ident arguments) instead of infoschema.ErrTableExists so callers retain the new MLog error contract.
♻️ Duplicate comments (1)
pkg/ddl/schematracker/dm_tracker.go (1)
233-235:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winImplement
SchemaTracker.GenerateMLogTableName.This still returns
"not implemented", so any schema-tracker/checker path that follows the newGenerateMLogTableName→CreateMaterializedViewLogflow fails immediately.🤖 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/schematracker/dm_tracker.go` around lines 233 - 235, Implement SchemaTracker.GenerateMLogTableName so it no longer returns the placeholder error: use the receiver (*SchemaTracker) to compute the materialized view log table name by invoking the existing CreateMaterializedViewLog flow (or the internal helper that derives MLog names) with the provided *ast.CreateMaterializedViewLogStmt and sessionctx.Context, and return the resulting name and any error; ensure the method signature matches GenerateMLogTableName(sessionctx.Context, *ast.CreateMaterializedViewLogStmt) (string, error) and propagate errors instead of returning "not implemented".
🤖 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.
Outside diff comments:
In `@pkg/ddl/schematracker/dm_tracker.go`:
- Around line 252-275: The existence check for mlog currently runs before
validating the base table, which masks the specific ddl errors; move the call to
d.TableClonedByName and the base-table guards (checks using baseTable.IsView(),
IsSequence(), TempTableType and baseTable.MaterializedViewBase.MLogID) to run
before invoking ddl.GetExistenceOfMLogTableNameChecker/mlogExists so you can
return ddl.ErrMLogAlreadyExists when the base table already has an MLog;
afterwards run the mlog existence check and, on name conflict, return the
specific ddl.ErrMLogTableNameConflict (using the same ast.Ident arguments)
instead of infoschema.ErrTableExists so callers retain the new MLog error
contract.
---
Duplicate comments:
In `@pkg/ddl/schematracker/dm_tracker.go`:
- Around line 233-235: Implement SchemaTracker.GenerateMLogTableName so it no
longer returns the placeholder error: use the receiver (*SchemaTracker) to
compute the materialized view log table name by invoking the existing
CreateMaterializedViewLog flow (or the internal helper that derives MLog names)
with the provided *ast.CreateMaterializedViewLogStmt and sessionctx.Context, and
return the resulting name and any error; ensure the method signature matches
GenerateMLogTableName(sessionctx.Context, *ast.CreateMaterializedViewLogStmt)
(string, error) and propagate errors instead of returning "not implemented".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7b28c68f-bd39-47b9-97e3-98524ff647a6
📒 Files selected for processing (7)
pkg/ddl/create_table.gopkg/ddl/executor.gopkg/ddl/materialized_view.gopkg/ddl/schematracker/dm_tracker.gopkg/executor/ddl.gopkg/executor/ddl_test.gopkg/executor/test/ddl/mview_log_ddl_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/executor/ddl.go
- pkg/ddl/executor.go
- pkg/ddl/materialized_view.go
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
1 participant
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.
What problem does this PR solve?
Issue Number: close #68840 #66768
Problem Summary:
What changed and how does it work?
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
Bug Fixes
Tests