DDL: support drop partition on the partitioned table with global indexes #19222

ldeng-ustc · 2020-08-16T16:17:34Z

What problem does this PR solve?

Support drop partition operation on the table which has global index.

Proposal: #18982

This PR is based on #18024 and parser #970.

What is changed and how it works?

What's Changed:

Drop partition is change to 4 state: public, delete only, delete reorg and none.

New ReorgIndexWorker interface to support different types of reorganization. Implement this interface in dropIndexWorker, use to clean up global indexes when dropping partitions.

Test for the new features.

How it Works:

From public to delete only, move the definitions of dropping partitions to DroppingDefinitions, then do nothing in state delete only. When ddl owner is in reorg state, all servers can not see the dropping partitions, so that owner can do reorganization safely.

In reorganization, interface ReorgIndexWorker receive task and return results.

// ReorgIndexWorker use to receive and handle index reorg tasks and return results.
type ReorgIndexWorker interface {
	Run(d *ddlCtx)
	Close()
	ID() int
	ResultCh() chan *reorgIndexResult
	TaskCh() chan *reorgIndexTask
}

Implement dropIndexWorker to clean up indexes. Move some code of addIndexWorker to baseIndexWorker , and modify it to support reorganizing multiple indexes to reuse the code.

Process of cleaning up the indexes is similar as adding index. cleanupGlobalIndexes clean the indexes partition by partition, cleanupPhysicalTableIndex split the handle range, and send task to dropIndexWorker, worker clean up the index entries related to handles in a txn, an return the result.

Potential problems

In previous TiDB, only add index has a real reorg state. So all reorganizing code is write for adding index. This PR modify these code to support cleaning up the global index in drop partition, it may cause some potential problems. It is hard to identify all problems, but I still list some of them here.

metric

3 metric variables (AddIndexProgress, AddIndexTotalCounter and BatchAddIdxHistogram) are used in reorganizing of add index. I leave them unmodified for the moment. So it may show wrong information when running reorganization of drop partition. These variables should be modified to support all reorganization works or we should add other 3 variables to monitor the drop partition process.

ddl job queue

Now, add index jobs use an independent ddl job queue so that long reorganizing time will not block other ddl jobs (see meta.AddIndexJobListKey). Maybe drop partion in partitioned table with global indexes should use the same queue or another new queue, but I am not sure.

Check List

Tests

Unit test

Release note

None.

tiancaiamao · 2020-08-21T06:23:39Z

Please resolve lock @ldeng-ustc

…partition

…op_partition

ddl/index.go

tiancaiamao · 2020-08-24T03:39:39Z

ddl/index.go

+		n := len(w.indexes)
+		for i, idxRecord := range idxRecords {
+			taskCtx.scanCount++
+			err := w.indexes[i%n].Delete(w.sessCtx.GetSessionVars().StmtCtx, txn, idxRecord.vals, idxRecord.handle)


What does i%n mean here?

For example, when dropping 3 indexes, records in idxRecords are belong to index1, index2, index3, index1, index2……

This is a good way to add/clean index records for multi-index reorg work.
For now, tidb will handle one element(index) every time, do them one by one, that's what the current element used for. If so we need some change in the reorgInfo, maybe we can keep running it as the old way and file an issue to accelerate all other reorg work like this. I will follow your pr continuously.

tiancaiamao · 2020-08-24T06:30:00Z

ddl/index.go

+		if len(idxWorkers) > int(workerCnt) {
+			workers := idxWorkers[workerCnt:]
+			idxWorkers = idxWorkers[:workerCnt]
+			closeReorgIndexWorkers(workers)


There is a defer closeReorgIndexWorkers(idxWorkers) at the begining of the function, will the worker be closed twice?

No, here idxWorkers is set to workers not closed.

ddl/index.go

tiancaiamao · 2020-08-24T06:39:05Z

ddl/index.go

+	if err != nil {
+		return false, errors.Trace(err)
+	}
+	reorg.StartHandle, reorg.EndHandle, reorg.PhysicalTableID = start, end, pid


Can add/drop index and drop partition DDL be run at the same time? if so, store this reorg information is insufficient, the DDL job will overwrite each other.

UpdateReorgMeta also save the DDL job ID, so it seems OK.

OK, the stored meta info would not overwrite each other.
But how about those two operations, I guess let them run at the same time is not safe.

https://github.com/pingcap/tidb/blob/master/ddl/ddl_worker.go#L170

ti-srebot · 2020-09-29T05:32:20Z

@ldeng-ustc, please update your pull request.

…partition

ti-srebot · 2020-10-09T16:22:42Z

@ldeng-ustc, please update your pull request.

ti-srebot · 2020-10-12T10:50:06Z

@tiancaiamao, @AilinKid, PTAL.

djshow832 · 2020-10-14T08:58:02Z

ddl/partition.go

+	return nt
+}
+
+func buildPlacementDropRules(schemaID, tableID int64, partitionIDs []int64) []*placement.RuleOp {


Where is it used?

djshow832 · 2020-10-14T09:04:50Z

ddl/partition.go

+		if d.infoHandle != nil {
+			bundles := make([]*placement.Bundle, 0, len(physicalTableIDs))
+			for _, ID := range physicalTableIDs {
+				oldBundle, ok := d.infoHandle.Get().BundleByName(placement.GroupID(ID))
+				if ok && !oldBundle.IsEmpty() {
+					bundles = append(bundles, buildPlacementDropBundle(ID))
+				}
+			}
+
+			err = infosync.PutRuleBundles(nil, bundles)
+			if err != nil {
+				job.State = model.JobStateCancelled
+				return ver, errors.Wrapf(err, "failed to notify PD the placement rules")
+			}
+		}


Duplicate code, it can be simplified.

AilinKid · 2020-10-14T08:02:11Z

ddl/index.go

+			rowDecoder:     rowDecoder,
+			defaultVals:    make([]types.Datum, len(t.WritableCols())),
+			rowMap:         make(map[int64]types.Datum, len(decodeColMap)),
+			metricCounter:  metrics.BackfillTotalCounter.WithLabelValues("add_idx_speed"),


maybe this metric is not suitable.

AilinKid · 2020-10-14T08:09:32Z

ddl/index.go

+			if err != nil {
+				return errors.Trace(err)
+			}
+			taskCtx.addedCount++


may the added count is not suitable here too

Maybe we should replace all "backfilling" in backfilling.go to "reorganize", because cleaning up index is not "backfilling", but it a big project, so I leave it unchanged now.

AilinKid · 2020-10-14T08:27:09Z

ddl/partition.go

 		// If an error occurs, it returns that it cannot delete all partitions or that the partition doesn't exist.
 		err = checkDropTablePartition(tblInfo, partNames)
 		if err != nil {
 			job.State = model.JobStateCancelled
 			return ver, errors.Trace(err)
 		}
-		physicalTableIDs = removePartitionInfo(tblInfo, partNames)
-	}
+		updateDroppingPartitionInfo(tblInfo, partNames)


how about move this after the placement bundle check below.

AilinKid · 2020-10-14T08:52:48Z

ddl/reorg.go

+	if job.SnapshotVer == 0 {
+		// For the case of the old TiDB version(do not exist the element information) is upgraded to the new TiDB version.
+		// Third step, we need to remove the element information to make sure we can save the reorganized information to storage.
+		failpoint.Inject("MockGetIndexRecordErr", func(val failpoint.Value) {


this failpoint is useless

AilinKid · 2020-10-14T08:53:31Z

ddl/reorg.go

+		job.SnapshotVer = ver.Ver
+		element = elements[0]
+	} else {
+		failpoint.Inject("MockGetIndexRecordErr", func(val failpoint.Value) {


AilinKid · 2020-10-14T09:20:17Z

ddl/index.go

+		n := len(w.indexes)
+		for i, idxRecord := range idxRecords {
+			taskCtx.scanCount++
+			err := w.indexes[i%n].Delete(w.sessCtx.GetSessionVars().StmtCtx, txn, idxRecord.vals, idxRecord.handle)


This is a good way to add/clean index records for multi-index reorg work.
For now, tidb will handle one element(index) every time, do them one by one, that's what the current element used for. If so we need some change in the reorgInfo, maybe we can keep running it as the old way and file an issue to accelerate all other reorg work like this. I will follow your pr continuously.

ti-srebot · 2020-10-16T09:22:40Z

@tiancaiamao, @AilinKid, @djshow832, PTAL.

…idb into global_index_drop_partition

ldeng-ustc · 2020-10-16T12:35:36Z

This is a good way to add/clean index records for multi-index reorg work.
For now, tidb will handle one element(index) every time, do them one by one, that's what the current element used for. If so we need some change in the reorgInfo, maybe we can keep running it as the old way and file an issue to accelerate all other reorg work like this. I will follow your pr continuously.

Thank you. In fact, I create this pr before element is added, so I did not use it. Maybe I will try to modify other reorg logic after finished works on global index.

djshow832

Rest LGTM

ddl/partition.go

Co-authored-by: djshow832 <zhangming@pingcap.com>

ti-srebot · 2020-10-18T14:18:11Z

@tiancaiamao, @AilinKid, @djshow832, PTAL.

djshow832

LGTM

djshow832 · 2020-10-19T02:24:21Z

/merge

ti-srebot · 2020-10-19T02:24:41Z

/run-all-tests

ldeng-ustc added 19 commits July 7, 2020 12:41

ddl: support add global index

47f82e1

Merge branch 'master' of github.com:pingcap/tidb

55b09e2

ddl: add config value 'EnableGlobalIndex'

58f90b7

Merge branch 'master' of github.com:pingcap/tidb into global_index

5d47ec6

fix

b15b1f2

tests: update testcase for creating global index

2e04b6c

go.mod: updade parser to support IndexInfo.Global

e5284c6

go.mod: updade parser to support IndexInfo.Global

2faf246

fix

505a620

Merge branch 'master' of github.com:pingcap/tidb into global_index

19b0a97

tablecodec: refactor GenIndexValue for extensibility

2febc3a

tablecodec: add decodeIndexKvGeneral for extensible index decoding

9ef4eab

tablecodec: use decodeIndexKvGeneral in DecodeIndexKV

c933af0

Merge branch 'master' of github.com:pingcap/tidb into global_index

5616d86

table: refine comments of index value layout

be23f72

fix comment

2ad6198

fix fmt

4d99cc3

ddl: support drop partition in the table with global index

7a06348

clean up code

53d9a11

ti-srebot added the contribution This PR is from a community contributor. label Aug 16, 2020

ldeng-ustc changed the title ~~DDL: support drop partition~~ DDL: support drop partition on the partitioned table with global indexes Aug 16, 2020

github-actions bot added component/config sig/sql-infra SIG: SQL Infra labels Aug 16, 2020

ldeng-ustc added 3 commits August 17, 2020 00:23

clean up

a70b358

Merge remote-tracking branch 'pingcap/master' into global_index

3dab738

Merge branch 'global_index' into global_index_drop_partition

8d635da

ldeng-ustc added 2 commits August 21, 2020 15:55

Merge remote-tracking branch 'pingcap/master' into global_index_drop_…

cdee33b

…partition

Merge branch 'master' of github.com:pingcap/tidb into global_index_dr…

2de2fd3

…op_partition

tiancaiamao reviewed Aug 24, 2020

View reviewed changes

ldeng-ustc added 2 commits October 3, 2020 00:03

Merge remote-tracking branch 'pingcap/master' into global_index_drop_…

f0e955e

…partition

fix check

ddd12da

lysu requested a review from AilinKid October 10, 2020 10:46

lysu added status/PTAL and removed status/require-change labels Oct 10, 2020

djshow832 reviewed Oct 14, 2020

View reviewed changes

AilinKid reviewed Oct 14, 2020

View reviewed changes

Merge branch 'master' into global_index_drop_partition

ff7f6a6

ldeng-ustc added 2 commits October 16, 2020 19:20

address comments

fb7faef

Merge branch 'global_index_drop_partition' of github.com:ldeng-ustc/t…

84900c0

…idb into global_index_drop_partition

djshow832 reviewed Oct 16, 2020

View reviewed changes

ddl/partition.go Outdated Show resolved Hide resolved

ldeng-ustc force-pushed the global_index_drop_partition branch from cc937d7 to b3d58c0 Compare October 17, 2020 12:13

address comment

d1ebeec

Co-authored-by: djshow832 <zhangming@pingcap.com>

ldeng-ustc force-pushed the global_index_drop_partition branch from b3d58c0 to d1ebeec Compare October 17, 2020 12:20

Merge branch 'master' into global_index_drop_partition

b4c8a01

djshow832 reviewed Oct 19, 2020

View reviewed changes

ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Oct 19, 2020

ti-srebot approved these changes Oct 19, 2020

View reviewed changes

ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Oct 19, 2020

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 19, 2020

ti-srebot merged commit 937949f into pingcap:master Oct 19, 2020

xhebox mentioned this pull request Aug 3, 2021

ddl: fix bug introduced by 19222 #26830

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DDL: support drop partition on the partitioned table with global indexes #19222

DDL: support drop partition on the partitioned table with global indexes #19222

ldeng-ustc commented Aug 16, 2020

tiancaiamao commented Aug 21, 2020

tiancaiamao Aug 24, 2020

ldeng-ustc Sep 6, 2020

AilinKid Oct 14, 2020 •

edited

Loading

tiancaiamao Aug 24, 2020

ldeng-ustc Sep 6, 2020

tiancaiamao Aug 24, 2020

ldeng-ustc Sep 6, 2020

tiancaiamao Sep 9, 2020

tiancaiamao Sep 9, 2020

ti-srebot commented Sep 29, 2020

ti-srebot commented Oct 9, 2020

ti-srebot commented Oct 12, 2020

djshow832 Oct 14, 2020

djshow832 Oct 14, 2020

AilinKid Oct 14, 2020

AilinKid Oct 14, 2020

ldeng-ustc Oct 16, 2020

AilinKid Oct 14, 2020

AilinKid Oct 14, 2020

AilinKid Oct 14, 2020

AilinKid Oct 14, 2020 •

edited

Loading

ti-srebot commented Oct 16, 2020

ldeng-ustc commented Oct 16, 2020

djshow832 left a comment

ti-srebot commented Oct 18, 2020

djshow832 left a comment

djshow832 commented Oct 19, 2020

ti-srebot commented Oct 19, 2020

DDL: support drop partition on the partitioned table with global indexes #19222

DDL: support drop partition on the partitioned table with global indexes #19222

Conversation

ldeng-ustc commented Aug 16, 2020

What problem does this PR solve?

What is changed and how it works?

Potential problems

Check List

Release note

tiancaiamao commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-srebot commented Sep 29, 2020

ti-srebot commented Oct 9, 2020

ti-srebot commented Oct 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

ti-srebot commented Oct 16, 2020

ldeng-ustc commented Oct 16, 2020

djshow832 left a comment

Choose a reason for hiding this comment

ti-srebot commented Oct 18, 2020

djshow832 left a comment

Choose a reason for hiding this comment

djshow832 commented Oct 19, 2020

ti-srebot commented Oct 19, 2020

AilinKid Oct 14, 2020 •

edited

Loading

AilinKid Oct 14, 2020 •

edited

Loading