redo(ticdc): add column info for redo ddl event (#12602)#12613
redo(ticdc): add column info for redo ddl event (#12602)#12613ti-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. |
|
@wk989898 This PR has conflicts, I have hold it. |
|
[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: ## 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. |
![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 column metadata to redo DDL events to improve recovery accuracy. Key changes include updating the RedoDDLEvent struct, modifying serialization logic, and enhancing the redo log reader to reconstruct table information. However, several critical issues were identified: the go.mod and go.sum files contain unresolved merge conflicts, and the ToRedoLog implementation fails to populate essential fields like TableName and Type, which will cause data inconsistency during recovery. Additionally, the redo reader uses a hardcoded timestamp for table metadata and employs log.Panic for error handling, which could lead to unnecessary process crashes.
| <<<<<<< HEAD | ||
| github.com/tikv/pd/client v0.0.0-20251219084741-029eb6e7d5d0 | ||
| github.com/tinylib/msgp v1.1.6 | ||
| ======= | ||
| github.com/tikv/pd/client v0.0.0-20260323032024-d7b638033a14 | ||
| github.com/tinylib/msgp v1.5.0 | ||
| >>>>>>> 5b45aa084a (redo(ticdc): add column info for redo ddl event (#12602)) |
There was a problem hiding this comment.
The go.mod file contains unresolved merge conflict markers. This will prevent the project from building and must be resolved before merging.
| <<<<<<< HEAD | ||
| github.com/tikv/pd/client v0.0.0-20251219084741-029eb6e7d5d0 h1:fguMZ4sSn/oLbUt+zDoNPd6+OE3Li4Rop2/3vFhu8lM= | ||
| github.com/tikv/pd/client v0.0.0-20251219084741-029eb6e7d5d0/go.mod h1:X3T+jK+4bLbDKgupmzvVXuySnCNV4Lfdm/bL8TAw3ik= | ||
| github.com/tinylib/msgp v1.1.6 h1:i+SbKraHhnrf9M5MYmvQhFnbLhAXSDWF8WWsuyRdocw= | ||
| github.com/tinylib/msgp v1.1.6/go.mod h1:75BAfg2hauQhs3qedfdDZmWAPcFMAvJE5b9rGOMufyw= | ||
| ======= | ||
| github.com/tikv/pd/client v0.0.0-20260323032024-d7b638033a14 h1:TVSx20m6DZMSiI37Dduu9RZb8yUvT1sgW8kCLAe+T5U= | ||
| github.com/tikv/pd/client v0.0.0-20260323032024-d7b638033a14/go.mod h1:4kxXuAQAREpH+lVbydVwGNNDmcwdj0RG4Ofwky08W/k= | ||
| github.com/tinylib/msgp v1.5.0 h1:GWnqAE54wmnlFazjq2+vgr736Akg58iiHImh+kPY2pc= | ||
| github.com/tinylib/msgp v1.5.0/go.mod h1:cvjFkb4RiC8qSBOPMGPSzSAx47nAsfhLVTCZZNuHv5o= | ||
| github.com/tjfoc/gmsm v1.3.2/go.mod h1:HaUcFuY0auTiaHB9MHFGCPx5IaLhTUd2atbCFBQXn9w= | ||
| github.com/tjfoc/gmsm v1.4.1 h1:aMe1GlZb+0bLjn+cKTPEvvn9oUEBlJitaZiiBwsbgho= | ||
| github.com/tjfoc/gmsm v1.4.1/go.mod h1:j4INPkHWMrhJb38G+J6W4Tw0AbuN8Thu3PbdVYhVcTE= | ||
| >>>>>>> 5b45aa084a (redo(ticdc): add column info for redo ddl event (#12602)) |
There was a problem hiding this comment.
The go.sum file contains unresolved merge conflict markers at multiple locations (e.g., lines 1012-1025 and 1384-1388). These must be resolved to ensure dependency integrity.
| var columns []*ColumnInfo | ||
| if d.TableInfo != nil && d.TableInfo.TableInfo != nil { | ||
| columns = make([]*ColumnInfo, 0, len(d.TableInfo.Columns)) | ||
| for _, col := range d.TableInfo.Columns { | ||
| columns = append(columns, &ColumnInfo{ | ||
| Name: col.Name.String(), | ||
| OriginDefaultValue: col.GetOriginDefaultValue(), | ||
| Type: col.GetType(), | ||
| Version: col.Version, | ||
| }) | ||
| } | ||
| } | ||
| return &RedoLog{ | ||
| RedoDDL: RedoDDLEvent{DDL: d}, | ||
| RedoDDL: RedoDDLEvent{DDL: d, Columns: columns}, | ||
| Type: RedoLogTypeDDL, | ||
| } |
There was a problem hiding this comment.
The ToRedoLog function fails to populate the TableName and Type fields in the RedoDDLEvent struct. These fields are essential for the redo log reader to correctly reconstruct the table metadata during recovery. Without them, the reader will receive empty table names and default types, which could lead to data inconsistency or errors during DDL application.
var columns []*ColumnInfo
var tableName TableName
if d.TableInfo != nil {
tableName = d.TableInfo.TableName
if d.TableInfo.TableInfo != nil {
columns = make([]*ColumnInfo, 0, len(d.TableInfo.Columns))
for _, col := range d.TableInfo.Columns {
columns = append(columns, &ColumnInfo{
Name: col.Name.String(),
OriginDefaultValue: col.GetOriginDefaultValue(),
Type: col.GetType(),
Version: col.Version,
})
}
}
}
return &RedoLog{
RedoDDL: RedoDDLEvent{
DDL: d,
Columns: columns,
TableName: tableName,
Type: d.Type,
},
Type: RedoLogTypeDDL,
}| } | ||
| columns = append(columns, colInfo) | ||
| } | ||
| return model.WrapTableInfo(0, ddl.TableName.Schema, 100, &timodel.TableInfo{ |
There was a problem hiding this comment.
The UpdateTS (third argument to WrapTableInfo) is hardcoded to 100. It is more appropriate to use the CommitTs from the DDL event to ensure the reconstructed metadata reflects the correct point in time.
| return model.WrapTableInfo(0, ddl.TableName.Schema, 100, &timodel.TableInfo{ | |
| return model.WrapTableInfo(0, ddl.TableName.Schema, ddl.DDL.CommitTs, &timodel.TableInfo{ |
| log.Panic("set origin default value failed", | ||
| zap.String("column", col.Name), | ||
| zap.Any("originDefaultValue", col.OriginDefaultValue), | ||
| zap.Error(err)) | ||
| } |
There was a problem hiding this comment.
Using log.Panic here will crash the entire TiCDC process if a DDL event in the redo log has an invalid default value. Consider returning an error instead to allow the system to handle the failure more gracefully (e.g., by stopping the specific replication task instead of the whole process).
|
@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. |
2 participants
This is an automated cherry-pick of #12602
What problem does this PR solve?
Issue Number: close #12600
What is changed and how it works?
Add column info for Redo DDL event to avoid data inconsistency if the column type is time default with origin_default
This PR #12490 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