store/gc_worker: Save gc_delete_range items that is done in another table instead of deleting them. #6512

MyonKeminta · 2018-05-09T08:10:48Z

Please refer to: tikv/tikv#3034
This pull request added a is_done field in mysql.gc_delete_range, and after deleting range, it marks the record in mysql.gc_delete_range as "Done" status rather than deleting it from the table.
Related pull request in TiKV: tikv/tikv#3046

At the same time this PR fix a bug that we incorrectly set element_id field unique in mysql.gc_delete_range. Element id of index is not globally unique.

Please be careful reviewing this PR

UPDATE: Save already-done gc_delete_range items in a new table.

MyonKeminta · 2018-05-09T10:00:59Z

@zhangjinpeng1987 PTAL

MyonKeminta · 2018-05-10T07:07:14Z

@tiancaiamao @coocood PTAL

zhangjinpeng87 · 2018-05-10T07:43:01Z

ddl/delete_range.go

@@ -35,7 +35,10 @@ import (
 )

 const (
-	insertDeleteRangeSQL = `INSERT IGNORE INTO mysql.gc_delete_range VALUES ("%d", "%d", "%s", "%s", "%d")`
+	// Use ON DUPLICATE KEY UPDATE to avoid already-processed records in the table breaking tests.


I think it is better to investigate why the test failed.

zhangjinpeng87 · 2018-05-10T07:44:59Z

ddl/util/util.go

@@ -27,8 +27,8 @@ import (
 )

 const (
-	loadDeleteRangeSQL     = `SELECT HIGH_PRIORITY job_id, element_id, start_key, end_key FROM mysql.gc_delete_range WHERE ts < %v ORDER BY ts`
-	completeDeleteRangeSQL = `DELETE FROM mysql.gc_delete_range WHERE job_id = %d AND element_id = %d`
+	loadDeleteRangeSQL     = `SELECT HIGH_PRIORITY job_id, element_id, start_key, end_key FROM mysql.gc_delete_range WHERE ts < %v AND is_done = FALSE ORDER BY ts`


ORDER BY ts is not necessary.

…deleterange-workaround-speedup

tiancaiamao · 2018-05-10T12:35:27Z

There is a currentBootstrapVersion , you should update its value to 21.
https://github.com/pingcap/tidb/pull/6512/files#diff-4a9e9120daccf5fc0d249b976f057938R306

shenli · 2018-05-10T12:55:07Z

In this case, the data in the gc_delete_range will increase forever?

it marks the record in mysql.gc_delete_range as "Done" status rather than deleting it from the table.

shenli · 2018-05-10T12:59:29Z

/run-all-tests

MyonKeminta · 2018-05-10T13:36:06Z

@shenli We plan to change it back later, after apply bool is implemented. For detail please see tikv/tikv#3034

…b.com/MyonKeminta/tidb into misono/deleterange-workaround-speedup

siddontang · 2018-05-10T18:57:31Z

@shenli this is just a workaround, we will fix this problem later.

tiancaiamao · 2018-05-11T03:12:13Z

LGTM

MyonKeminta · 2018-05-11T05:29:39Z

@zimulala @coocood PTAL

zhangjinpeng87 · 2018-05-14T01:14:21Z

@shenli PTAL

shenli · 2018-05-14T02:55:52Z

/run-all-tests

coocood · 2018-05-14T03:04:18Z

session/bootstrap.go

+
+	doReentrantDDL(s, "ALTER TABLE mysql.gc_delete_range DROP INDEX element_id", ddl.ErrCantDropFieldOrKey)
+	doReentrantDDL(s, "ALTER TABLE mysql.gc_delete_range DROP INDEX job_id", ddl.ErrCantDropFieldOrKey)
+	doReentrantDDL(s, "ALTER TABLE mysql.gc_delete_range ADD UNIQUE INDEX delete_range_index (job_id, element_id)", ddl.ErrDupKeyName)


Can we add index first?
If we drop the element id unique index, then two records inserted with the same element_id, then add unique index would fail.

coocood · 2018-05-14T03:08:03Z

ddl/util/util.go

@@ -89,8 +90,14 @@ func LoadDeleteRanges(ctx sessionctx.Context, safePoint uint64) (ranges []DelRan
 // CompleteDeleteRange deletes a record from gc_delete_range table.


Please update the comment

…b.com/MyonKeminta/tidb into misono/deleterange-workaround-speedup

coocood · 2018-05-14T04:01:53Z

LGTM

MyonKeminta · 2018-05-14T05:24:56Z

@coocood @zimulala PTAL again..

tiancaiamao · 2018-05-14T07:19:07Z

LGTM @zimulala

zimulala · 2018-05-14T07:54:48Z

ddl/util/util.go

-	completeDeleteRangeSQL = `DELETE FROM mysql.gc_delete_range WHERE job_id = %d AND element_id = %d`
-	updateDeleteRangeSQL   = `UPDATE mysql.gc_delete_range SET start_key = "%s" WHERE job_id = %d AND element_id = %d AND start_key = "%s"`
+	loadDeleteRangeSQL        = `SELECT HIGH_PRIORITY job_id, element_id, start_key, end_key FROM mysql.gc_delete_range WHERE ts < %v`
+	recordDoneDeletedRangeSQL = `INSERT IGNORE INTO mysql.gc_delete_range_done SELECT * FROM mysql.gc_delete_range WHERE job_id = %d AND element_id = %d`


Why need "IGNORE" ?

The insert statement in ddl/delete_range.go has IGNORE too...
It's might considering that maybe multiple TiDBs are trying to insert the record

zimulala

LGTM

zimulala · 2018-05-14T09:16:34Z

/run-all-tests

zimulala · 2018-05-14T11:00:49Z

/run-unit-test

BusyJay · 2018-05-14T11:31:13Z

Ping

…f deleting them. (pingcap#6512)

…f deleting them. (#6512) (#6568)

MyonKeminta added 3 commits May 9, 2018 15:57

Use new field is_done rather than deleting the entry

a8bc5a4

Fix tests

056a4f1

Add comment for the ON DUPLICATE UPDATE clause

22b876c

MyonKeminta changed the title ~~[WIP] store/gc_worker: Use is_done flag field instead of deleting entry from gc_delete_range table~~ store/gc_worker: Use is_done flag field instead of deleting entry from gc_delete_range table May 9, 2018

MyonKeminta changed the title ~~store/gc_worker: Use is_done flag field instead of deleting entry from gc_delete_range table~~ [DNM] store/gc_worker: Use is_done flag field instead of deleting entry from gc_delete_range table May 9, 2018

MyonKeminta mentioned this pull request May 9, 2018

raftstore/apply: Temporarily skip delete all in range tikv/tikv#3046

Merged

MyonKeminta changed the title ~~[DNM] store/gc_worker: Use is_done flag field instead of deleting entry from gc_delete_range table~~ store/gc_worker: Use is_done flag field instead of deleting entry from gc_delete_range table May 10, 2018

zhangjinpeng87 reviewed May 10, 2018

View reviewed changes

MyonKeminta added 6 commits May 10, 2018 16:02

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

fff6bf7

…deleterange-workaround-speedup

On duplicate key do not update

5f22da6

Fix incorrect unique key

48e9bc8

Address comments

462a2c5

Rename the index

c811861

Fix test

00eab9d

Merge branch 'master' into misono/deleterange-workaround-speedup

1070391

MyonKeminta added 2 commits May 10, 2018 21:37

increase currentBootstrapVersion

c6dbddb

Merge branch 'misono/deleterange-workaround-speedup' of https://githu…

84b2526

…b.com/MyonKeminta/tidb into misono/deleterange-workaround-speedup

MyonKeminta added 2 commits May 11, 2018 08:25

Fix that delete range occuring during upgrading may crash

bb0dcdc

Move add column statement to first

caf8536

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label May 11, 2018

MyonKeminta changed the title ~~[DNM] store/gc_worker: Use is_done flag field instead of deleting entry from gc_delete_range table~~ [DNM] store/gc_worker: Save gc_delete_range items that is done in another table instead of deleting them. May 14, 2018

Create new table first or it will got into error executing drop index

21b1986

MyonKeminta changed the title ~~[DNM] store/gc_worker: Save gc_delete_range items that is done in another table instead of deleting them.~~ store/gc_worker: Save gc_delete_range items that is done in another table instead of deleting them. May 14, 2018

Merge branch 'master' into misono/deleterange-workaround-speedup

de456c8

coocood reviewed May 14, 2018

View reviewed changes

MyonKeminta added 2 commits May 14, 2018 11:21

Address comments

7cce6c2

Merge branch 'misono/deleterange-workaround-speedup' of https://githu…

f53c31b

…b.com/MyonKeminta/tidb into misono/deleterange-workaround-speedup

tiancaiamao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 14, 2018

Merge branch 'master' into misono/deleterange-workaround-speedup

825dd14

zimulala reviewed May 14, 2018

View reviewed changes

zimulala approved these changes May 14, 2018

View reviewed changes

Merge branch 'master' into misono/deleterange-workaround-speedup

fa3b107

zimulala added all-tests-passed status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels May 14, 2018

tiancaiamao merged commit 10e57a5 into pingcap:master May 14, 2018

MyonKeminta added a commit to MyonKeminta/tidb that referenced this pull request May 15, 2018

*: save gc_delete_range items that is done in another table instead o…

322d150

…f deleting them. (pingcap#6512)

disksing pushed a commit that referenced this pull request May 16, 2018

*: save gc_delete_range items that is done in another table instead o…

cdabfc9

…f deleting them. (#6512) (#6568)

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store/gc_worker: Save gc_delete_range items that is done in another table instead of deleting them. #6512

store/gc_worker: Save gc_delete_range items that is done in another table instead of deleting them. #6512

MyonKeminta commented May 9, 2018 •

edited

MyonKeminta commented May 9, 2018

MyonKeminta commented May 10, 2018

zhangjinpeng87 May 10, 2018

zhangjinpeng87 May 10, 2018

tiancaiamao commented May 10, 2018

shenli commented May 10, 2018

shenli commented May 10, 2018

MyonKeminta commented May 10, 2018

siddontang commented May 10, 2018

tiancaiamao commented May 11, 2018

MyonKeminta commented May 11, 2018

zhangjinpeng87 commented May 14, 2018

shenli commented May 14, 2018

coocood May 14, 2018

coocood May 14, 2018

coocood commented May 14, 2018

MyonKeminta commented May 14, 2018

tiancaiamao commented May 14, 2018 •

edited

zimulala May 14, 2018

MyonKeminta May 14, 2018 •

edited

zimulala left a comment

zimulala commented May 14, 2018

zimulala commented May 14, 2018

BusyJay commented May 14, 2018

		@@ -89,8 +90,14 @@ func LoadDeleteRanges(ctx sessionctx.Context, safePoint uint64) (ranges []DelRan
		// CompleteDeleteRange deletes a record from gc_delete_range table.

store/gc_worker: Save gc_delete_range items that is done in another table instead of deleting them. #6512

store/gc_worker: Save gc_delete_range items that is done in another table instead of deleting them. #6512

Conversation

MyonKeminta commented May 9, 2018 • edited

MyonKeminta commented May 9, 2018

MyonKeminta commented May 10, 2018

zhangjinpeng87 May 10, 2018

Choose a reason for hiding this comment

zhangjinpeng87 May 10, 2018

Choose a reason for hiding this comment

tiancaiamao commented May 10, 2018

shenli commented May 10, 2018

shenli commented May 10, 2018

MyonKeminta commented May 10, 2018

siddontang commented May 10, 2018

tiancaiamao commented May 11, 2018

MyonKeminta commented May 11, 2018

zhangjinpeng87 commented May 14, 2018

shenli commented May 14, 2018

coocood May 14, 2018

Choose a reason for hiding this comment

coocood May 14, 2018

Choose a reason for hiding this comment

coocood commented May 14, 2018

MyonKeminta commented May 14, 2018

tiancaiamao commented May 14, 2018 • edited

zimulala May 14, 2018

Choose a reason for hiding this comment

MyonKeminta May 14, 2018 • edited

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented May 14, 2018

zimulala commented May 14, 2018

BusyJay commented May 14, 2018

MyonKeminta commented May 9, 2018 •

edited

tiancaiamao commented May 14, 2018 •

edited

MyonKeminta May 14, 2018 •

edited