redo: add column info for redo ddl event#4809
redo: add column info for redo ddl event#4809wk989898 wants to merge 6 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
the
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded persistent per-column metadata to DDL redo logs: introduced exported Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ti-chi-bot
bot
added
the
size/XXL
![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 introduces a ColumnInfo struct and adds a Columns field to DDLEventInRedoLog to store column metadata in redo logs, addressing a bug related to timestamp default values in different time zones. The changes include updating the ToRedoLog and ToDDLEvent methods to handle this new metadata, along with generated serialization code and tests. A potential nil pointer dereference was identified in the ToRedoLog function when accessing TableInfo before a nil check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/event/redo.go`:
- Around line 219-227: The code in ToRedoLog() accesses d.TableInfo.GetColumns()
and builds columns before checking d.TableInfo for nil, causing a panic for DDL
events without table metadata; guard the access by first verifying d.TableInfo
!= nil (and optionally d.TableInfo.GetColumns() != nil) before iterating, and
only construct the columns slice and append ColumnInfo entries when d.TableInfo
is non-nil; refer to d.TableInfo, ToRedoLog(), and ColumnInfo to locate and fix
the offending block.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7132d7da-c277-4fdd-beb0-cfcfda7fefc4
📒 Files selected for processing (3)
pkg/common/event/redo.gopkg/common/event/redo_gen.gopkg/common/event/redo_gen_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/common/event/redo.go (1)
394-418:⚠️ Potential issue | 🟠 MajorHandle older redo payloads without
columns.Line 394 still treats
r.DDL.Columns == nilas “table with zero columns”. For redo files written before this field existed, replay will silently build an incompleteTableInfoinstead of failing fast or version-gating the format, so the applier is still not semantically backward-compatible with persisted pre-change redo logs.Suggested direction
func (r *RedoDDLEvent) ToDDLEvent() *DDLEvent { blockedTables := r.DDL.BlockedTables blockedTableNames := r.DDL.BlockedTableNames if blockedTables == nil { blockedTables = &InfluencedTables{InfluenceType: InfluenceTypeNormal} blockedTableNames = []SchemaTableName{{SchemaName: r.TableName.Schema, TableName: r.TableName.Table}} } + if r.DDL.Columns == nil && r.TableName.Table != "" { + log.Panic("redo ddl column metadata missing", + zap.String("schema", r.TableName.Schema), + zap.String("table", r.TableName.Table), + zap.Uint64("commitTs", r.DDL.CommitTs)) + } columns := make([]*timodel.ColumnInfo, 0, len(r.DDL.Columns))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/event/redo.go` around lines 394 - 418, The code currently treats r.DDL.Columns == nil as an empty column list which silently accepts older redo payloads; update the DDLEvent construction to detect a nil r.DDL.Columns and fail fast (return an error or log.Panic) instead of building a TableInfo with zero columns. Specifically, in the block that iterates r.DDL.Columns (used to build columns and call commonType.NewTableInfo4Decoder and create the DDLEvent), add an explicit nil check for r.DDL.Columns and emit a clear error message indicating an unsupported/older redo format (include identifying info like r.TableName and any available redo version metadata) so replaying pre-change redo logs does not produce an invalid TableInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/common/event/redo.go`:
- Around line 394-418: The code currently treats r.DDL.Columns == nil as an
empty column list which silently accepts older redo payloads; update the
DDLEvent construction to detect a nil r.DDL.Columns and fail fast (return an
error or log.Panic) instead of building a TableInfo with zero columns.
Specifically, in the block that iterates r.DDL.Columns (used to build columns
and call commonType.NewTableInfo4Decoder and create the DDLEvent), add an
explicit nil check for r.DDL.Columns and emit a clear error message indicating
an unsupported/older redo format (include identifying info like r.TableName and
any available redo version metadata) so replaying pre-change redo logs does not
produce an invalid TableInfo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 884b9299-2968-4bcf-be28-98ccd4fea24a
📒 Files selected for processing (2)
pkg/common/event/redo.gopkg/common/event/redo_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/common/event/redo.go (1)
390-410:⚠️ Potential issue | 🟠 MajorDistinguish missing
Columnsfrom a real zero-column schema.On Lines 390-410,
r.DDL.Columns == niltakes the same path as an intentionally empty column list and silently rebuilds an emptyTableInfo. Older redo payloads decode exactly that way when the new field is absent, so pre-change redo files still replay without the metadata this fix depends on. Please version-gate the format or fail fast whenColumnsis missing instead of continuing with zero columns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/event/redo.go` around lines 390 - 410, The code treats r.DDL.Columns == nil the same as an intentionally empty column list, which hides older redo payloads that omitted the field; update the handler to detect a nil Columns slice and fail-fast or enforce a format version check before rebuilding TableInfo: when r.DDL.Columns == nil (use the r.DDL or envelope version field or add one), return/raise a clear error mentioning r.TableName and that Columns is missing (instead of constructing an empty []*timodel.ColumnInfo), or gate the path behind a version check so only payloads with the new schema proceed to build timodel.ColumnInfo via SetOriginDefaultValue and commonType.NewTableInfo4Decoder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/event/redo.go`:
- Around line 406-410: The code is calling the test-only helper
commonType.NewTableInfo4Decoder from redo replay; replace this with the
production initializer for table metadata (i.e., use the supported
constructor/factory that the package exposes for runtime use instead of
NewTableInfo4Decoder). Update the creation of tableInfo in redo.go by invoking
the production API (for example the package's NewTableInfo /
NewTableInfoFromTiTable or equivalent factory that accepts schema, ID, name and
columns) and populate it with r.TableName.Schema, r.TableName.TableID,
ast.NewCIStr(r.TableName.Table) and columns so redo replay no longer depends on
the test-only NewTableInfo4Decoder.
---
Duplicate comments:
In `@pkg/common/event/redo.go`:
- Around line 390-410: The code treats r.DDL.Columns == nil the same as an
intentionally empty column list, which hides older redo payloads that omitted
the field; update the handler to detect a nil Columns slice and fail-fast or
enforce a format version check before rebuilding TableInfo: when r.DDL.Columns
== nil (use the r.DDL or envelope version field or add one), return/raise a
clear error mentioning r.TableName and that Columns is missing (instead of
constructing an empty []*timodel.ColumnInfo), or gate the path behind a version
check so only payloads with the new schema proceed to build timodel.ColumnInfo
via SetOriginDefaultValue and commonType.NewTableInfo4Decoder.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: feba35b4-36da-4206-9537-e34d7fb7bfb3
📒 Files selected for processing (1)
pkg/common/event/redo.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/common/event/redo.go (1)
390-405: Consider setting column ID for consistency with DML reconstruction.The DML reconstruction in
ToDMLEvent()(line 297) setsID: int64(idx)for each column, but this DDL path omits the column ID. If downstream code relies onColumnInfo.ID, this could cause unexpected behavior.♻️ Proposed fix to set column ID
for _, col := range r.DDL.Columns { colInfo := &timodel.ColumnInfo{ + ID: int64(len(columns)), Name: ast.NewCIStr(col.Name), State: timodel.StatePublic, Version: col.Version, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/event/redo.go` around lines 390 - 405, The DDL reconstruction loop builds []*timodel.ColumnInfo from r.DDL.Columns but never sets ColumnInfo.ID, causing inconsistency with ToDMLEvent() which sets ID: int64(idx); update the loop that creates colInfo in redo.go to assign colInfo.ID = int64(idx) (use the loop index) before appending to columns so DDL-created ColumnInfo objects have the same ID semantics as DML reconstruction and downstream code that reads ColumnInfo.ID works correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/common/event/redo.go`:
- Around line 390-405: The DDL reconstruction loop builds []*timodel.ColumnInfo
from r.DDL.Columns but never sets ColumnInfo.ID, causing inconsistency with
ToDMLEvent() which sets ID: int64(idx); update the loop that creates colInfo in
redo.go to assign colInfo.ID = int64(idx) (use the loop index) before appending
to columns so DDL-created ColumnInfo objects have the same ID semantics as DML
reconstruction and downstream code that reads ColumnInfo.ID works correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7e0fb72-9826-4ddf-b778-9c20fe3aaf68
📒 Files selected for processing (1)
pkg/common/event/redo.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand 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 #4699
What is changed and how it works?
Add column info for Redo DDL event to avoid panic if the column type is time default with origin_default
This PR #3991 will use column info in the DDL event to align the upstream current timestamp. TiCDC has to encode column info for redo ddl event, and redo apply will decode the column info for replicating the ddl event.
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
New Features
Bug Fixes
Tests