ddl: fix alter table exchange partition does not work if table has tiflash replica #17940
Conversation
merge from master
merge tidb from master
Codecov Report
@@ Coverage Diff @@
## master #17940 +/- ##
===========================================
Coverage 79.5854% 79.5854%
===========================================
Files 535 535
Lines 144578 144578
===========================================
Hits 115063 115063
Misses 20276 20276
Partials 9239 9239 |
| @@ -2844,14 +2844,37 @@ func checkFieldTypeCompatible(ft *types.FieldType, other *types.FieldType) bool | |||
| return true | |||
| } | |||
|
|
|||
| func checkTiFlashReplicaCompatible(source *model.TiFlashReplicaInfo, target *model.TiFlashReplicaInfo) bool { | |||
Co-authored-by: Lynn <zimu_xia@126.com>
Co-authored-by: Lynn <zimu_xia@126.com>
ddl/partition.go
Outdated
| @@ -812,6 +812,19 @@ func (w *worker) onExchangeTablePartition(d *ddlCtx, t *meta.Meta, job *model.Jo | |||
| // exchange table meta id | |||
| partDef.ID = nt.ID | |||
|
|
|||
| // Clear the tiflash replica available status. | |||
| if pt.TiFlashReplica != nil { | |||
| pt.TiFlashReplica.Available = false | |||
There was a problem hiding this comment.
why set Available to false? I think if exchange partition is allowed, the TiFlash replica status of both table are always the same(both available or both not available), it is not necessary to reset the TiFlash replica status to false if they are already available.
There was a problem hiding this comment.
One more question: is it possible that the available flag is changed between checkTiFlashReplicaCompatible and onExchangeTablePartition , if yes, then we need handle it more carefully.
There was a problem hiding this comment.
I get that It seem that tiflash now doesnot support exchange partition in slack. So I just reset false like truncate partition. I would delete the code about changing tiflash status.
There was a problem hiding this comment.
TiFlash does not support exchange partition when #17149 is merged, and it has supported now, so you can safely delete related code, but take care of my last question, if available flag is changed, we need to handle the status carefully for both partition table and non-partition table.
There was a problem hiding this comment.
I hava a question about how to change a available status when a exchange partition happening ? It seems that only one worker handle ddl events.
There was a problem hiding this comment.
I hava a question about how to change a
availablestatus when aexchange partitionhappening ? It seems that only one worker handle ddl events.
I'm not very familiar with TiDB, if available status can not be changed during exchange partition, then just ignore my question.
ddl/partition.go
Outdated
| @@ -812,6 +812,17 @@ func (w *worker) onExchangeTablePartition(d *ddlCtx, t *meta.Meta, job *model.Jo | |||
| // exchange table meta id | |||
| partDef.ID = nt.ID | |||
|
|
|||
| // Clear the tiflash replica available status. | |||
There was a problem hiding this comment.
should remove this comment since the code no longer clear the tiflash replica available status
ddl/partition.go
Outdated
| @@ -812,6 +812,17 @@ func (w *worker) onExchangeTablePartition(d *ddlCtx, t *meta.Meta, job *model.Jo | |||
| // exchange table meta id | |||
| partDef.ID = nt.ID | |||
|
|
|||
| // Clear the tiflash replica available status. | |||
| if pt.TiFlashReplica != nil { | |||
| // Set partition replica become unavailable. | |||
|
/run-unit-test |
|
LGTM |
|
@windtalker, Thanks for your review, however we are sorry that your vote won't be count. You are not a reviewer or committer or co-leader or leader for the related sigs:ddl(slack). |
zimulala
added
the
status/can-merge
|
@zimulala,Thanks for you review. |
|
/run-all-tests |
What problem does this PR solve?
Issue Number: close ##17918
Problem Summary:
We need to compare tiflash replica information and reset the tiflash replica status when exchanging partition.
Check List
Tests
Side effects
Release note