br: Compatibility problem between br and TiDB for DDLs about attribute in Incremental Backup

[joccau]

What problem does this PR solve?

Because TiDB V5.3.0 support TableAttributes and TablePartitionAttributes.
Incremental Backup need skip these DDLs to be compatible with previous br versions.

Issue Number: close #29359

Problem Summary:

What is changed and how it works?

Check List

Tests

• Manual test (add detailed scripts or steps below)

Test steps

• Make full backup in the cluster of v5.3.0-alpha TiKV with v5.3.0-alpha br
• Do DDL sql alter table t attributes ”merge_option=allow”
• Make incremental backup in the cluster of V5.3.0
• Make full restore in the cluster of v5.0.0 TiKV with v5.0.0 br
• Make incremental restore in the cluster of V5.0.0

All of above steps succeed

Release note

Incremental Backup need skip DDLs about attribute for compatibility

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• 3pointer
• kennytm

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

[joccau]

/cc @kennytm @3pointer @YuJuncen

[joccau]

/sig migrate

[joccau]

/component br

[kennytm]

This solution will always skip the attributes even in 5.3 backup → 5.3 restore scenario. I don't think it is a good long-term solution (maybe not even good as a short-term solution).

The ideal way is to copy what pingcap/parser#1287 has done. However, BR fetches the DDL query from the *model.Job structure, whose Query string is filled in using the statement's OriginText() 😱, so fixing Restore() isn't gonna help.

tidb/ddl/ddl.go

Lines 542 to 543 in fff946c

	// Get a global job ID and put the DDL job in the queue.
	job.Query, _ = ctx.Value(sessionctx.QueryString).(string)

tidb/session/session.go

Line 1657 in fff946c

se.SetValue(sessionctx.QueryString, s.OriginText())

(will changing OriginText() to Restore() going to introduce subtle breakages?)

[3pointer]

This solution will always skip the attributes even in 5.3 backup → 5.3 restore scenario. I don't think it is a good long-term solution (maybe not even good as a short-term solution).

The ideal way is to copy what pingcap/parser#1287 has done. However, BR fetches the DDL query from the *model.Job structure, whose Query string is filled in using the statement's OriginText() 😱, so fixing Restore() isn't gonna help.

tidb/ddl/ddl.go

Lines 542 to 543 in fff946c

	// Get a global job ID and put the DDL job in the queue.
	job.Query, _ = ctx.Value(sessionctx.QueryString).(string)

tidb/session/session.go

Line 1657 in fff946c

se.SetValue(sessionctx.QueryString, s.OriginText())

(will changing OriginText() to Restore() going to introduce subtle breakages?)

another alternative is we handle the parser error during restore, we build a white list and skip some errors in white list

[YuJuncen]

@kennytm I guess we can parse and the restore the query with format.RestoreTiDBSpecialComment when replaying the DDL job?

(Or just do the procedure when we backing up?)

[joccau]

This solution will always skip the attributes even in 5.3 backup → 5.3 restore scenario. I don't think it is a good long-term solution (maybe not even good as a short-term solution).
The ideal way is to copy what pingcap/parser#1287 has done. However, BR fetches the DDL query from the *model.Job structure, whose Query string is filled in using the statement's OriginText() 😱, so fixing Restore() isn't gonna help.

tidb/ddl/ddl.go

Lines 542 to 543 in fff946c

	// Get a global job ID and put the DDL job in the queue.
	job.Query, _ = ctx.Value(sessionctx.QueryString).(string)

tidb/session/session.go

Line 1657 in fff946c

se.SetValue(sessionctx.QueryString, s.OriginText())

(will changing OriginText() to Restore() going to introduce subtle breakages?)

another alternative is we handle the parser error during restore, we build a white list and skip some errors in white list

we should fix it in v5.0.0 with the method of "skip some errors in white list when restore", and do nothing in v5.3.0.

[joccau]

/unhold

[kennytm]

IMO either we claim that the 5.3 backup won't work on 5.0, or make the 5.3 output compatible with 5.0. Any solution requiring changing 5.0 is no good, why would the user upgrade the existing 5.0 cluster.

[joccau]

/run-integration-br-test

[kennytm]

rest lgtm

you might need a test case 🤔.

[joccau]

/run-integration-br-test

[YuJuncen]

/merge

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: 91cbade

[joccau]

/cherrypick release-5.3

[rleungx]

/label needs-cherry-pick-5.3

[ti-srebot]

cherry pick to release-5.3 in PR #29493