DNM: Release 6.5.12 Hotfix 20260420#12616
Conversation
This reverts commit 6ae60b0.
<!-- Thank you for contributing to TiFlow! Please read MD's [CONTRIBUTING](https://github.com/pingcap/tiflow/blob/master/CONTRIBUTING.md) document **BEFORE** filing this PR. --> <!-- Please create an issue first to describe the problem. There MUST be one line starting with "Issue Number: " and linking the relevant issues via the "close" or "ref". For more info, check https://pingcap.github.io/tidb-dev-guide/contribute-to-tidb/contribute-code.html#referring-to-an-issue. --> Issue Number: close #xxx - Unit test - Integration test - Manual test (add detailed scripts or steps below) - No code ```release-note Please refer to [Release Notes Language Style Guide](https://pingcap.github.io/tidb-dev-guide/contribute-to-tidb/release-notes-style-guide.html) to write a quality release note. If you don't think this PR needs a release note then fill it with `None`. ``` Reviewed-on: https://git.pingcap.net/pingkai/tiflow/pulls/17 Reviewed-by: zhaoyilin <zhaoyilin@pingcap.cn> Co-authored-by: 何芊蔚 <qianwei.he@pingcap.cn> Co-committed-by: 何芊蔚 <qianwei.he@pingcap.cn>
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
contribution
|
Hi @Debra-He. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
ti-chi-bot
Bot
added
the
needs-ok-to-test
|
Welcome @Debra-He! |
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 enhances DDL synchronization by tracking partition IDs during truncation, ensuring DDL ordering via sequence numbers, and implementing session timestamp management in the MySQL sink to preserve upstream CURRENT_TIMESTAMP values. It also adds logic to wait for source table DDLs during CREATE TABLE ... LIKE operations. Review feedback identifies a potential nil pointer dereference in partition info retrieval, incomplete parsing logic for specific ALTER TABLE statements, lack of support for numeric timezone offsets, and precision loss issues when converting nanosecond timestamps to float64.
| newPi := tbInfo.GetPartitionInfo() | ||
| oldPi := old.GetPartitionInfo() | ||
| newPartitionIDMap := make(map[int64]struct{}, len(newPi.Definitions)) |
There was a problem hiding this comment.
Potential panic if newPi is nil. While TRUNCATE TABLE on a partitioned table should normally result in a new partitioned table, tbInfo is derived from the DDL job's metadata which might be incomplete or malformed in certain edge cases. A safety check for newPi is necessary before accessing its fields.
| newPi := tbInfo.GetPartitionInfo() | |
| oldPi := old.GetPartitionInfo() | |
| newPartitionIDMap := make(map[int64]struct{}, len(newPi.Definitions)) | |
| newPi := tbInfo.GetPartitionInfo() | |
| if newPi == nil { | |
| return cerror.ErrSnapshotTableNotFound.GenWithStack("table %d is not a partition table", tbInfo.ID) | |
| } | |
| oldPi := old.GetPartitionInfo() | |
| newPartitionIDMap := make(map[int64]struct{}, len(newPi.Definitions)) |
| case ast.AlterTableAddColumns, ast.AlterTableModifyColumn, ast.AlterTableChangeColumn, ast.AlterTableAlterColumn: | ||
| for _, col := range spec.NewColumns { | ||
| if hasCurrentTimestampDefault(col) { | ||
| cols[col.Name.Name.L] = struct{}{} | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for AlterTableStmt is incomplete. It includes ast.AlterTableAlterColumn in the case list but then only iterates over spec.NewColumns, which is typically empty for AlterTableAlterColumn. This case is used for ALTER TABLE ... ALTER COLUMN ... SET DEFAULT ..., and the column name and options should be extracted from spec.Column and spec.Options respectively.
case ast.AlterTableAddColumns, ast.AlterTableModifyColumn, ast.AlterTableChangeColumn:
for _, col := range spec.NewColumns {
if hasCurrentTimestampDefault(col) {
cols[col.Name.Name.L] = struct{}{}
}
}
case ast.AlterTableAlterColumn:
for _, opt := range spec.Options {
if opt.Tp == ast.ColumnOptionDefaultValue && isCurrentTimestampExpr(opt.Expr) {
cols[spec.Column.Name.L] = struct{}{}
}
}| return time.UTC, nil | ||
| } | ||
| tz := strings.Trim(timezone, "\"") | ||
| return time.LoadLocation(tz) |
There was a problem hiding this comment.
time.LoadLocation only works for named timezones (e.g., "UTC", "Asia/Shanghai"). If the timezone string is a numeric offset (e.g., "+08:00"), which is a valid value for TiDB's @@time_zone, it will return an error. Consider using a more robust utility that handles both named zones and numeric offsets.
| for _, f := range formats { | ||
| t, err := time.ParseInLocation(f, val, loc) | ||
| if err == nil { | ||
| return float64(t.UnixNano()) / float64(time.Second), nil |
There was a problem hiding this comment.
Using float64 to represent Unix timestamps in nanoseconds leads to precision loss. A float64 has 53 bits of mantissa, which can only represent about 15-17 significant decimal digits, while a Unix timestamp in nanoseconds requires 19 digits. This will likely corrupt the microsecond part of the timestamp when setting @@timestamp. It is recommended to use time.Time throughout these helper functions and format it directly to a string using fmt.Sprintf("%d.%06d", t.Unix(), t.Nanosecond()/1000) to preserve microsecond precision.
|
/test pull-verify |
4 participants
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