-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ddl: Exchange partition rollback #45877
Merged
ti-chi-bot
merged 16 commits into
pingcap:master
from
mjonss:exchange-partition-rollback-45791
Aug 10, 2023
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
638e273
Added tests
mjonss 7d80c9d
Fixed version, without cleanup
mjonss 00bd267
Updated to be backwards compatible (in case of non-processed diffs).
mjonss d324142
Linting
mjonss 50c666b
Minor test fix
mjonss bab23d9
Better error handling, with rollback only when neccessary
mjonss bb371a4
Moved check of placement rules before changing table structures
mjonss 4c27d02
minor cleanup
mjonss 1b5c7c6
Linting
mjonss b42cb1c
Test cleanup
mjonss 6dbdd81
Removed a comment
mjonss 98584ec
Linting
mjonss 04f7908
Added Placement Policy tests
mjonss 1aaf9eb
Added test for tidb/issues/45920
mjonss 8402932
Removed debug line in test
mjonss ccae6d3
Merge remote-tracking branch 'pingcap/master' into exchange-partition…
mjonss File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4746,7 +4746,6 @@ func checkExchangePartition(pt *model.TableInfo, nt *model.TableInfo) error { | |
return errors.Trace(dbterror.ErrPartitionExchangeForeignKey.GenWithStackByArgs(nt.Name)) | ||
} | ||
|
||
// NOTE: if nt is temporary table, it should be checked | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was already checked, in |
||
return nil | ||
} | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1373,24 +1373,28 @@ | |
diff.OldSchemaID = oldSchemaIDs[0] | ||
diff.AffectedOpts = affects | ||
case model.ActionExchangeTablePartition: | ||
var ( | ||
ptSchemaID int64 | ||
ptTableID int64 | ||
partName string | ||
withValidation bool | ||
) | ||
err = job.DecodeArgs(&diff.TableID, &ptSchemaID, &ptTableID, &partName, &withValidation) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the partitionID as diff.TableID most likely caused #45920 ! |
||
if err != nil { | ||
return 0, errors.Trace(err) | ||
} | ||
diff.OldTableID = job.TableID | ||
affects := make([]*model.AffectedOption, 1) | ||
affects[0] = &model.AffectedOption{ | ||
SchemaID: ptSchemaID, | ||
TableID: ptTableID, | ||
OldTableID: ptTableID, | ||
diff.OldSchemaID = job.SchemaID | ||
if job.SchemaState != model.StatePublic { | ||
diff.TableID = job.TableID | ||
diff.SchemaID = job.SchemaID | ||
} else { | ||
// Update the partitioned table (it is only done in the last state) | ||
var ( | ||
ptSchemaID int64 | ||
ptTableID int64 | ||
ptDefID int64 // Not needed, will reload the whole table | ||
partName string // Not used | ||
withValidation bool // Not used | ||
) | ||
// See ddl.ExchangeTablePartition | ||
err = job.DecodeArgs(&ptDefID, &ptSchemaID, &ptTableID, &partName, &withValidation) | ||
if err != nil { | ||
return 0, errors.Trace(err) | ||
} | ||
diff.SchemaID = ptSchemaID | ||
diff.TableID = ptTableID | ||
} | ||
diff.AffectedOpts = affects | ||
case model.ActionTruncateTablePartition: | ||
diff.TableID = job.TableID | ||
if len(job.CtxVars) > 0 { | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can add some cases in
TestExchangePartitionWithPlacement
in the fileplacement_policy_test.go
so that you can check some placement bundle settings are also set correctly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I just added more complete tests there.