ddl: implement reorganize for table partition when adding index #6814
Conversation
tiancaiamao
force-pushed
the
reorg-table-partition
branch
from
2789afb
to
9de4d72
Compare
tiancaiamao
force-pushed
the
reorg-table-partition
branch
from
d58920d
to
4c9c970
Compare
tiancaiamao
force-pushed
the
reorg-table-partition
branch
from
29b0a3e
to
3990e81
Compare
tiancaiamao
changed the title
ddl/reorg.go
Outdated
| // PartitionID returns the partition ID of the reorgInfo, or table ID if it's not a partition. | ||
| func (r *reorgInfo) PartitionID() int64 { | ||
| ret := r.Job.ID | ||
| if r.Job.ReorgMeta != nil && r.Job.ReorgMeta.PartitionID != 0 { |
There was a problem hiding this comment.
s/r.Job.ReorgMeta.PartitionID != 0/ r.Job.ReorgMeta.PartitionID != r.Job.ID @tiancaiamao .
There was a problem hiding this comment.
job.ID has nothing to do with TableID or PartitionID @ciscoxll
There was a problem hiding this comment.
@tiancaiamao You look at the code, if the normal table GetPartitionInfo gets a failure result, what might happen?
tiancaiamao
added
require-LGT3
ddl/ddl_db_test.go
Outdated
| } | ||
|
|
||
| s.tk.MustExec("alter table t add index idx (hired)") | ||
| // TODO: Make admin check table work for partition. |
There was a problem hiding this comment.
I'll fix this TODO after this PR is merged https://github.com/pingcap/tidb/pull/6963/files,
because admin check table would benefit from that refactor.
ddl/index.go
Outdated
| idx := -1 | ||
| for i, def := range defs { | ||
| if partitionID == def.ID { | ||
| idx = i |
There was a problem hiding this comment.
Why not use "ID" directly?
There was a problem hiding this comment.
What do you mean by "ID" ?
There was a problem hiding this comment.
See the comment of this function:
// findNextPartitionID finds the next partition ID in the PartitionDefinition.
// Returns -1 if this partitionID is the last one.
This function find the next partition ID, using offset is more convenient. @zimulala
There was a problem hiding this comment.
Could we write it as follows code?
if partitionID == def.ID {
if i == len(defs)-1{
return 0
}else{
return def[i+1].ID
}
}
|
LGTM |
| @@ -684,9 +683,11 @@ func (w *addIndexWorker) handleBackfillTask(d *ddlCtx, task *reorgIndexTask) *ad | |||
| result := &addIndexResult{addedCount: 0, nextHandle: handleRange.startHandle, err: nil} | |||
| lastLogCount := 0 | |||
| startTime := time.Now() | |||
| index := tables.NewIndex(task.partitionID, w.table.Meta(), w.indexInfo) | |||
There was a problem hiding this comment.
task.partitionID confuses me, we need to do a refactor after this PR.
There was a problem hiding this comment.
@shenli is doing that.
ddl/reorg.go
Outdated
| } | ||
|
|
||
| func (r *reorgInfo) UpdateHandle(txn kv.Transaction, handle int64) error { | ||
| func (r *reorgInfo) UpdateHandle(txn kv.Transaction, startHandle, endHandle, partitionID int64) error { |
There was a problem hiding this comment.
Could we name the UpdateHandle to UpdateReorgMeta, or something else?
…nto reorg-table-partition
|
PTAL @winkyao |
| @@ -149,7 +149,7 @@ func (w *worker) runReorgJob(t *meta.Meta, reorgInfo *reorgInfo, lease time.Dura | |||
| // Update a job's RowCount. | |||
| job.SetRowCount(rowCount) | |||
| // Update a reorgInfo's handle. | |||
| err := t.UpdateDDLReorgHandle(job, doneHandle) | |||
| err := t.UpdateDDLReorgStartHandle(job, doneHandle) | |||
There was a problem hiding this comment.
Shall we update the endHandle and the partitionID herer?
There was a problem hiding this comment.
If we not, when we are adding partition id 2 indices, and we crashed, then the startHandle set to 1000, then after we restart the job, we will get previous partition id, and the incorrect startHandle.
There was a problem hiding this comment.
This code is compatible to the old one.
The reorgInfo will not change to another partition automatically, the only place to do it is in function updateReorgInfo.
I think update both the endHandle and the partitionID here would bring more uncertain things.
There was a problem hiding this comment.
So maybe we can call reorgInfo.UpdateReorgMeta in handleReorgTasks when the err is nil?
|
/run-all-tests |
ciscoxll
added
status/LGT3
What have you changed? (mandatory)
handletostartHandle, endHandle, partitionIDPlease NOTE that:
This PR changes the DDL reorganize meta key, reviewer should be careful with the rolling update!
What are the type of the changes (mandatory)?
How has this PR been tested (mandatory)?
unit test
Add a few positive/negative examples (optional)