Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
[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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the schema store's DDL event processing for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ti-chi-bot
bot
added
the
size/M
📝 WalkthroughWalkthroughChanges add logic to reconstruct CREATE TABLE statements when multi-table DDL events have mismatched query counts. A new helper function attempts to split queries, detects mismatches, and rebuilds queries using SHOW CREATE TABLE results via executor, with a corresponding test verifying this reconstruction behavior. Changes
Sequence DiagramsequenceDiagram
participant Handler as DDL Handler
participant Splitter as Query Splitter
participant Comparator as Count Validator
participant Executor as TiDB Executor
participant Result as Result Builder
Handler->>Splitter: SplitQueries(rawEvent)
Splitter-->>Handler: parsed queries
Handler->>Comparator: Compare counts
alt Count matches
Comparator-->>Result: Use original queries
else Count mismatches
Comparator->>Executor: For each table, ConstructResultOfShowCreateTable
Executor-->>Executor: Generate CREATE TABLE from metadata
Executor-->>Result: Rebuilt query per table
end
Result-->>Handler: Final queries
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ 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 |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
logservice/schemastore/persist_storage_ddl_handlers.go (1)
2797-2832: The query reconstruction logic is well-structured.The
splitCreateTablesQueriesfunction handles the edge case where the original query cannot be properly split to match the table count. The approach of usingConstructResultOfShowCreateTableto rebuild queries from table info is reasonable.A few observations:
- The variable name
querysshould conventionally bequeries(proper English plural), though this is consistent with usage elsewhere in the file.- Good defensive check for nil
tableInfoat line 2816-2818.- Proper error wrapping with
cerror.WrapError.Minor: Consider consistent naming with English plural
-func splitCreateTablesQueries(rawEvent *PersistedDDLEvent) ([]string, error) { - querys, err := commonEvent.SplitQueries(rawEvent.Query) - if err == nil && len(querys) == len(rawEvent.MultipleTableInfos) { - return querys, nil +func splitCreateTablesQueries(rawEvent *PersistedDDLEvent) ([]string, error) { + queries, err := commonEvent.SplitQueries(rawEvent.Query) + if err == nil && len(queries) == len(rawEvent.MultipleTableInfos) { + return queries, nil } fields := []zap.Field{ - zap.Int("queryCount", len(querys)), + zap.Int("queryCount", len(queries)), zap.Int("tableCount", len(rawEvent.MultipleTableInfos)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logservice/schemastore/persist_storage_ddl_handlers.go` around lines 2797 - 2832, In splitCreateTablesQueries rename the misspelled local variable querys to queries (and update all uses within the function including the len(querys) checks and zap.Int("queryCount", ...) reference) to use the correct English plural; ensure you update the early-return return queries, nil and the rebuiltQuerys logic so names are consistent and compile cleanly, keeping all error handling and behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Line 2813: Replace the test-only mocked context usage: remove
mock.NewContext() and instead obtain a real session/context for production use
(for example, create a session from the TiDB domain/session pool or use
session.CreateSession and its returned context) so that
ConstructResultOfShowCreateTable() can access real session state (e.g.,
ctx.GetSessionVars().SQLMode); alternatively, if this handler is for external
callers, execute SHOW CREATE TABLE as SQL and use that result rather than
calling ConstructResultOfShowCreateTable() with a mocked ctx. Ensure the new
code uses the domain/session pool API to create or borrow a session, sets any
required SessionVars, calls ConstructResultOfShowCreateTable() (or runs SHOW
CREATE TABLE) with that real context, and properly closes/returns the session.
---
Nitpick comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 2797-2832: In splitCreateTablesQueries rename the misspelled local
variable querys to queries (and update all uses within the function including
the len(querys) checks and zap.Int("queryCount", ...) reference) to use the
correct English plural; ensure you update the early-return return queries, nil
and the rebuiltQuerys logic so names are consistent and compile cleanly, keeping
all error handling and behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45d1d5f3-fdc9-42bb-ae81-9207a76979d0
📒 Files selected for processing (2)
logservice/schemastore/persist_storage_ddl_handlers.gologservice/schemastore/persist_storage_test.go
| } | ||
| log.Warn("create tables query count not match table count rebuild queries by table info", fields...) | ||
|
|
||
| ctx := mock.NewContext() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TiDB mock.NewContext usage in production code ConstructResultOfShowCreateTable
💡 Result:
mock.NewContext() (from github.com/pingcap/tidb/util/mock) is intended for tests only, not production use. The package docs explicitly say: “Package mock is just for test only.” [1]
ConstructResultOfShowCreateTable(ctx sessionctx.Context, ...) depends on real session state (e.g., it reads ctx.GetSessionVars().SQLMode, escapes identifiers accordingly, etc.), so a mocked context can silently produce wrong output or miss required behavior/config. [3]
What to do instead (production):
- If you’re outside TiDB (normal app code): don’t call this internal helper—just run SQL:
SHOW CREATE TABLE ...against TiDB. - If you’re inside TiDB/embedding TiDB: use a real session (
sessionctx.Context) created by TiDB’s session creation flow (new connections callsession.CreateSession(); internal ops typically obtain sessions fromdomain’s session pool), rather thanutil/mock. [2]
Sources
- TiDB
util/mockpackage docs (test-only) — (pkg.go.dev) - TiDB dev guide: how real sessions are created/obtained — (pingcap.github.io)
ConstructResultOfShowCreateTableusesctx.GetSessionVars().SQLModeetc. — (seaxiang.com)
Citations:
- 1: https://pkg.go.dev/github.com/pingcap/tidb/util/mock
- 2: https://pingcap.github.io/tidb-dev-guide/understand-tidb/session.html
- 3: https://www.seaxiang.com/blog/05a44662bb4f4b7095db6cd01757cd17
Remove mock.NewContext() from production code path.
mock.NewContext() from github.com/pingcap/tidb/util/mock is explicitly documented as test-only. Using it in production code is unsafe because ConstructResultOfShowCreateTable() depends on real session state (e.g., ctx.GetSessionVars().SQLMode) that a mocked context does not provide. This can silently produce incorrect results.
Replace with a real session created via TiDB's session creation flow (e.g., from domain's session pool for internal operations, or run SHOW CREATE TABLE as SQL for external callers).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@logservice/schemastore/persist_storage_ddl_handlers.go` at line 2813, Replace
the test-only mocked context usage: remove mock.NewContext() and instead obtain
a real session/context for production use (for example, create a session from
the TiDB domain/session pool or use session.CreateSession and its returned
context) so that ConstructResultOfShowCreateTable() can access real session
state (e.g., ctx.GetSessionVars().SQLMode); alternatively, if this handler is
for external callers, execute SHOW CREATE TABLE as SQL and use that result
rather than calling ConstructResultOfShowCreateTable() with a mocked ctx. Ensure
the new code uses the domain/session pool API to create or borrow a session,
sets any required SessionVars, calls ConstructResultOfShowCreateTable() (or runs
SHOW CREATE TABLE) with that real context, and properly closes/returns the
session.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of handling CREATE TABLES DDLs by introducing a fallback mechanism. When splitting the DDL query string fails or results in a mismatched number of queries, it now reconstructs the CREATE TABLE statements from the table metadata. This is a solid improvement that makes the system more resilient to unexpected DDL query formats. I've added one suggestion to make the fallback logic even more robust.
| query := queryBuilder.String() | ||
| if !strings.HasSuffix(query, ";") { | ||
| query += ";" | ||
| } |
There was a problem hiding this comment.
The current logic for appending a semicolon is not robust against trailing whitespace. If queryBuilder.String() were to return a query with a semicolon followed by spaces (e.g., "CREATE TABLE ...; "), this code would append another semicolon. While executor.ConstructResultOfShowCreateTable is unlikely to produce such output, making this fallback logic more robust by trimming whitespace is a good practice.
| query := queryBuilder.String() | |
| if !strings.HasSuffix(query, ";") { | |
| query += ";" | |
| } | |
| query := strings.TrimSpace(queryBuilder.String()) | |
| if !strings.HasSuffix(query, ";") { | |
| query += ";" | |
| } |
1 participant
What problem does this PR solve?
Issue Number: close #xxx
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit
Bug Fixes
Tests